search

biocro / biocro

https://biocro.github.io/
Other
41 stars 19 forks source link

Update framework to v1.1.2 #66

Closed eloch216 closed 8 months ago

eloch216 commented 9 months ago

This new version of the framework fixes some mistakes related to the use abs in our code.

eloch216 commented 9 months ago

To my mind, this update is entirely unnecessary, since it is std::abs that is actually the correct usage.

Okay, I understand now.

I just double-checked, and there are lots of test failures on Windows using R v3.6.0 if there are any unqualified abs in the code (even if there is an #include <cmath> directive). This issue can be fixed by replacing abs with either fabs or std::abs. I figured out the former solution, but didn't realize that the latter one was also an option. (And I didn't know that it is more correct to use std::abs.)

I will change the framework back to std::abs. Although it seems silly, I think the appropriate git-flow method for doing this will be to make a new version (1.1.3) that's pretty much identical to version 1.1.1. I will also replace all instances of fabs by std::abs in the module code.

gsrohde commented 9 months ago

I will change the framework back to std::abs. Although it seems silly, I think the appropriate git-flow method for doing this will be to make a new version (1.1.3) that's pretty much identical to version 1.1.1.

Since this PR was never merged into develop or main, since those branches have never been tied to any framework version beyond 1.1.1 (and similarly with skelBML), I certainly would be tempted to force-revert the biocro/framework branches to how they were before hotfix-v1.1.2 was merged into develop and main, in essence pretending that those merges never happened. It seems unlikely that any users would be affected by doing so.

Still, there are biocro/framework pull requests 7 and 8 on record as having been merged, so this would likely be confusing to anyone engaging in "forensics", and rather than dealing with this and any other possible ramifications of a force-revert that I may not have thought of, I think the "silly" solution is probably safest, and ultimately, simpler.

justinmcgrath commented 9 months ago

I think the rewriting history gets confusing and should only be used as a last resort, or if it's done only on your local repository and doesn't affect anyone else. Recording our mistakes is more straightforward than trying to act like they didn't happen.

eloch216 commented 9 months ago

This PR is now actually updating the framework to version 1.1.3

eloch216 commented 9 months ago

BTW, I think we might be ready to make a new release after this PR is merged. We have (mostly) addressed the R CMD check warnings and removed references to ebimodeling, which seems worthy of a new release.

eloch216 commented 9 months ago

Looks like there was another harmonic oscillator test failure on Ubuntu with R v3.6.0:

── Failure ('test.HarmonicOscillationModeling.R:164:13'): Harmonic oscillator position and velocity values match the expected values (random parameters and initial values) ──
  result$velocity[index] (`actual`) not equal to result$expected_velocity[index] (`expected`).

  actual != expected but don't know how to show the difference

I will try re-running the test. Still not sure if this is something to be worried about.