Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and generalize differentiate #32

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

greimel
Copy link

@greimel greimel commented May 6, 2022

I have rewritten differentiate. First, I've split up the functions. Second, I've removed the @generated. Third, I've added a three state version. I think it would be quite easy to make it one function for arbitrary dimensional state spaces.

I changed the bound for the julia version, because I am using the (; a) == (a = a, ) syntax.

Correctness

  • Tests pass locally
  • Since you don't have any tests that check numbers (do you?), I kept the old functions and left the code I used to test the equivalence of the functions
  • I left some comments in the cross_difference function, where I believe the wrong Delta is used at the boundaries. I've left it consistent with the current implementation

To do

  • I've not yet tried to 3-d code
  • I will add an example to the examples folder

@matthieugomez
Copy link
Owner

Hey! I need to look at this more closely. That being said, i would really like to avoid adding two new dependences.

@greimel
Copy link
Author

greimel commented May 7, 2022

Sure! I'll leave them for more convenient developing until you are happy with the rest.

@matthieugomez
Copy link
Owner

matthieugomez commented May 19, 2022

Okay, I am back from holidays. Before I look into it,

  • I think it would be useful to remove the two new dependences, so that I can look at the final version directly.
  • Did you check benchmarks? I do not care that much about the first run, i.e. compile time, but I do care about the second run.

Let me know when you want me to look into it (by de-drafting your PR).

@matthieugomez
Copy link
Owner

matthieugomez commented May 19, 2022

For your cross_difference function, you may be right. Could you explain a bit more what you want to change (and why)?

@azev77
Copy link

azev77 commented Jun 22, 2022

@greimel has there been any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants