Skip to content

update README: WeakValueDicts unmaintained - #2364

Closed
juliasloan25 wants to merge 0 commit into
mainfrom
js/readme
Closed

update README: WeakValueDicts unmaintained#2364
juliasloan25 wants to merge 0 commit into
mainfrom
js/readme

Conversation

@juliasloan25

Copy link
Copy Markdown
Member

WeakValueDicts.jl is no longer maintained and is not threadsafe. ClimaCore.jl doesn't currently depend on that package, but previous versions do. This PR updates the README to explain that those older versions are unsupported and shouldn't be used.

See discussion in twavv/WeakValueDicts.jl#2 (comment) for more details.

@nefrathenrici nefrathenrici left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@juliasloan25

Copy link
Copy Markdown
Member Author

Tests here are failing after only changing the README, likely because of the new RootSolvers v0.4.5 release. Tagging @szy21 @tapios for visibility

@imreddyTeja

Copy link
Copy Markdown
Member

Tests here are failing after only changing the README, likely because of the new RootSolvers v0.4.5 release. Tagging @szy21 @tapios for visibility

I think the latest release dropped the default max iterations by a factor of 10.

@szy21

szy21 commented Aug 4, 2025

Copy link
Copy Markdown
Member

It fails at initialization though. Maybe we need to clear the depot?

@szy21

szy21 commented Aug 4, 2025

Copy link
Copy Markdown
Member

Oh I see that the unit tests are failing - yes that is probably from the RootSolvers

@tapios

tapios commented Aug 4, 2025

Copy link
Copy Markdown
Member

Tests here are failing after only changing the README, likely because of the new RootSolvers v0.4.5 release. Tagging @szy21 @tapios for visibility

I think the latest release dropped the default max iterations by a factor of 10.

I think the issue is an early convergence check, not the iteration count. Patch on the way.

@tapios

tapios commented Aug 4, 2025

Copy link
Copy Markdown
Member

Tests here are failing after only changing the README, likely because of the new RootSolvers v0.4.5 release. Tagging @szy21 @tapios for visibility

I think the latest release dropped the default max iterations by a factor of 10.

I think the issue is an early convergence check, not the iteration count. Patch on the way.

Actually, no, the issue is an unreasonable requirement to get a floating point precision match between the computed and expected mesh spacing. Yes, maybe with 10,000 iterations we can get there, but it really does not make sense to test that. Also if higher accuracy is desired, this should not be handled by default choices in the RootSolvers but by custom choices made locally.

@akshaysridhar Can you please replace the tests here (in tests/interval.jl) by tests with isapprox and rtol = 1e-3 (or whatever makes sense, 1e-3 iseems to be used in src/interval.jl)?

@szy21

szy21 commented Aug 4, 2025

Copy link
Copy Markdown
Member

Should we add downstream tests in RootSolvers?

@tapios

tapios commented Aug 4, 2025

Copy link
Copy Markdown
Member

Should we add downstream tests in RootSolvers?

I prefer not to. RootSolvers is a very basic package that is not geared toward any particular CliMA (or other) package. We should make sure that edge cases are properly handled, and we should add tests covering them to RootSolvers when we find them. But it does not make sense in my view to bloat the dependencies of RootSolver testing. Here, for example, it's noa RootSolvers problem from what I can see, but a problem of inappropriate tests.

@akshaysridhar

Copy link
Copy Markdown
Member

Tests here are failing after only changing the README, likely because of the new RootSolvers v0.4.5 release. Tagging @szy21 @tapios for visibility

I think the latest release dropped the default max iterations by a factor of 10.

I think the issue is an early convergence check, not the iteration count. Patch on the way.

Actually, no, the issue is an unreasonable requirement to get a floating point precision match between the computed and expected mesh spacing. Yes, maybe with 10,000 iterations we can get there, but it really does not make sense to test that. Also if higher accuracy is desired, this should not be handled by default choices in the RootSolvers but by custom choices made locally.

@akshaysridhar Can you please replace the tests here (in tests/interval.jl) by tests with isapprox and rtol = 1e-3 (or whatever makes sense, 1e-3 iseems to be used in src/interval.jl)?

I'll review these tests and make the changes today.

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.

7 participants