Skip to content

Fix/type stability - #57

Merged
bgctw merged 5 commits into
EarthyScience:mainfrom
olivierbonte:fix/type_stability
Aug 4, 2025
Merged

Fix/type stability#57
bgctw merged 5 commits into
EarthyScience:mainfrom
olivierbonte:fix/type_stability

Conversation

@olivierbonte

Copy link
Copy Markdown
Contributor

Suggestions to improve type stability of some functions. .

Comment thread src/unit_conversions.jl Outdated
@bgctw

bgctw commented Aug 1, 2025

Copy link
Copy Markdown
Member

Thanks Oliver for those suggestions. Can you, please, change some of the hard constructors to the suggested usage of convert?, And can you, please, resolve the conflict in unit_conversions.jl, before I merge it to main?

@olivierbonte

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, conflict is resolved and as as suggested changed use of FT(...) to convert(FT,...).

@bgctw

bgctw commented Aug 1, 2025

Copy link
Copy Markdown
Member

For constants I prefer the FT(...) over the convert, then the Julia compiler inserts the correctly typed value directly in the compiled code, e.g. FT(0.3). If you you are not bored by this picky issue, please, change those back to the more readable hard cast. Otherwise, I can merge it the way it is.

@bgctw
bgctw merged commit c93d9b9 into EarthyScience:main Aug 4, 2025
4 checks passed
@olivierbonte

Copy link
Copy Markdown
Contributor Author

Sorry for late reply, was planning on changing back to FT(...) now, but I see the PR is already merged. Still made the commit olivierbonte@eabb4be. Made a new PR just in case you still want to adapt it to be this way, otherwise feel free to decline the new PR, found at #58

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.

2 participants