-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
PINNErrorVsTime Benchmark Updates #1159
base: master
Are you sure you want to change the base?
Conversation
@ChrisRackauckas I got an error on running the iterations which said that the maxiters are less than 1000 so I set all maxiters to 1100. Actually the decision was a bit arbitrary but is that a good number ?? |
https://docs.sciml.ai/SciMLBenchmarksOutput/v0.5/PINNErrorsVsTime/diffusion_et/ It's supposed to just show the error over time and then get cutoff. I don't see why making it longer would help. |
Yes. Actually I set it to that number just to get rid of that error |
Wait what's the error? |
the error went off on running again |
what error? |
maxiters should be a number greater than 1000 |
can you please just show the error... |
|
I see, that's for the sampling algorithm. You should only need that on Cuhre? |
Yes. But as Cuhre was the first one in the line I thought setting to 1100 just for it would not solve the problem, so I set it to 1100 for all of them |
The CI has passed here. And all the code seems to run perfectly. Can you please review ?? |
@ArnoStrouwen SciML/Integrals.jl#124 can you remind me what the purpose behind this was? |
I don't remember myself, but that PR links to: |
Uninitialized memory in the original C: giordano/Cuba.jl#12 (comment) fantastic stuff numerical community, that's your classic method that everyone says when they say "all of the old stuff is robust" 😅 |
Can you force latest majors and make sure the manifest resolves? |
I bump forced the latest versions and resolved manifests but initially there were a lot of version conflicts. I removed IntegralsCuba and IntegralsCubature for a while to resolve them. The manifest resolved but adding both of them back again poses some more version conflicts |
Can you share the resolution errors? |
@ChrisRackauckas These are the resolution errors that occur |
Oh those were turned into extensions. Change |
Sure !! 🫡 |
Should I consider running this again @ChrisRackauckas ?? |
@ChrisRackauckas This value comes from QuasiRandomTraining + BFGS algorithm |
It is just one time point? It's definitely an outlier. You got rid of the maximum time part: put that back? |
No. It's an array of two values: But I told you the maximum among the two. I haven't removed the maximum time part from the implementation. It's there but the whole code takes around 30 hours to execute for all the algorithms. I just ran this code for the most suspected outlier |
Any suggestions on further changes on this implementation @ChrisRackauckas ?? |
Training with QuasiRandomTraining: Training with QuadratureTraining CubaCuhre(): Training with Quadrature Training HCubatureJL() Training Quadrature Training CubatureJLh() Training with Quadrature Training CubaturwJLp() Training with GridTraining: Training with Stochastic Training: |
So I must remove them, right ?? |
@ChrisRackauckas Any further code changes required here ?? Are we ready to merge this ?? |
That plot hasn't changed. |
Yeah. So What parameters should I consider for making the changes exactly ?? |
The times array ?? or should I remove the outliers ?? or some algorithms ? |
The previous version had time caps to fix this. They should probably be re-added. |
Dict{Any, Any} with 14 entries: @ChrisRackauckas This is the new output after adding time caps |
Is there something under the legend? Move it out |
Oh! I got why this was happening, actually the code is correct but the graphs were appearing too wonky or small due to the number of iterations being performed |
@ChrisRackauckas Your views on this ? Is this how it should be ?? |
@ChrisRackauckas Could you please review ?? |
Can you log the t there? |
Added the code to log t for each algorithm |
@ChrisRackauckas All checks have passed the graph still appears the same though 😅 . Could you please review ?? I have logged t there like you said |
@ChrisRackauckas probably the graph is a bit zoomed out ?? |
@ChrisRackauckas Further changes to the code doesn't seem to change the graph much. Any more tweaks you might suggest ?? |
rest the code works fine and updated to the latest bump |
Are we ready to merge this ?? |
|
You keep pinging but the plot still isn't fixed. |
Yes. The plot isn't fixed. The reason why I was pinging was to know if there are some alternate methods to fix this, because I have tried a lot of variations of this still observed no significant changes. And every push I make on the repo, takes 48+ hours to pass through the CI and the results turn out to be something I didn't expect or get when I ran them locally. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.