Skip to content

Conversation

@JordiBolibar
Copy link
Member

@JordiBolibar JordiBolibar commented Apr 11, 2025

PR for #150.

Changes implemented so far:

  • Making the ODEProblem local instead of global
  • Specifying a bit argument types.

Things I have tried:

  • Adding parametric types to forward.jl functions. Since ReverseDiff uses TrackedArrays and values it just breaks everything. The abstract types are the most specific thing I managed to narrow down.
  • Using EnzymeVJP(): this gave an ugly error which was not super straightforward to debug. Once we gain more experience with ODINN we can try to tackle this.

@facusapienza21 the benchmark has failed. In local it seems to work, so I'm not sure what failed.

We can work on this iteratively.

JordiBolibar and others added 3 commits April 8, 2025 10:55
Making the `ODEProblem` local instead of global, and specifying a bit argument types.
@JordiBolibar JordiBolibar added the enhancement New feature or request label Apr 11, 2025
@facusapienza21
Copy link
Member

Hi @JordiBolibar ! Thank you for taking a look at this! definitively some improvement in the code!

I am happy to see what break, but a few points:

  • Notice that the repo style now follows the Julia standards and you are now breaking the quality check format. This is not critical for exploration, but I will prefer not to merge into main something that does not pass the check.
  • Can you make a PR to the branch upPerformance instead? I created this branch to follow up on this.
  • I now agree with what Alban say that is better to work in branch on the same repo that having the PR from the fork: I could directly jump in into your branch and make the changes to fix the PR, but since this is coming from your fork I cannot directly commit there.

Copy link
Member

@facusapienza21 facusapienza21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small comments on the code, other than the comments I previously posted.

@JordiBolibar JordiBolibar changed the base branch from main to upPerformance April 14, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants