EconPDEs.jl icon indicating copy to clipboard operation
EconPDEs.jl copied to clipboard

Simplify and generalize `differentiate`

Open greimel opened this issue 3 years ago • 5 comments
trafficstars

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

greimel avatar May 06 '22 08:05 greimel

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

matthieugomez avatar May 07 '22 20:05 matthieugomez

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

greimel avatar May 07 '22 20:05 greimel

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 avatar May 19 '22 11:05 matthieugomez

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

matthieugomez avatar May 19 '22 12:05 matthieugomez

@greimel has there been any progress on this?

azev77 avatar Jun 22 '22 18:06 azev77