-
Notifications
You must be signed in to change notification settings - Fork 37
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
Redefine species.mass as a list of floats #344
Redefine species.mass as a list of floats #344
Conversation
I agree that this seems more a bug (and I am trying to take into account my bias for not wanting to bump major version numbers.) Edit: also as far as I know, no one we know of has implemented mass yet? Hey, we should keep an errata somewhere about "known bugs in 1.0.0" until we actually release an 1.0.1 that addresses them. But I agree with merging this. In the errata we could point out that a workaround for this bug while retaining full 1.0.0 compatibility is to define different species for the different isotopes and then instead use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job finding this and writing it up as a PR. I've looked it over and don't see any issues (apart from the discussion of backwards compatibility).
That's not true. It's part of the OPTIMADE Python tools, hence part of the Materials Project, ODBX, and Materials Cloud.
This sounds like a great idea! |
Same bias here :)
I think a changelog (as suggested in #197) could cater for this purpose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very sensible fix 👍 😃 Thanks @merkys !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this @merkys! Happy to accept, pending the changes suggested by @CasperWA (just ping me another review request if I need to re-accept)
I think a changelog (as suggested in #197) could cater for this purpose.
+1 for a rolling changelog that includes known/fixed bugs, I don't think a GH wiki page is visible enough (feels more like an internal knowledge base). I'm happy to spin up a manual one for now, though we might also consider auto-generating one based on merged PRs.
I think we really need to decide on our branching in #340 too...
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Done!
For starters, manual changelog would be just great!
Agree. Consensus for that was not a strong one (from my perspective), thus I'd be happy to hear other opinions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks @merkys !
Let's move the changelog/bug list tracking discussion to an appropriate issue.
Since there were no opposition to the status of a "bug fix", I have merged the PR. Regarding #340: this PR should appear in both the next patch and the next minor release. |
When looking into means to differentiate hydrogen, deuterium and tritium (see #327), I found out that information about isotopes in multi-species sites can easily be lost, because
species.mass
defined per-site, not per-species. Thus I suggest redefiningspecies.mass
as a list of floats.Redefinition brings a problem of backwards-incompatibility. Here are ways to solve it:
species.mass
with its current definition, and makespecies.masses
a list of floats. This retains backwards compatibility, but does not sound good (all subproperties ofspecies
are in singular). Another suggestion isspecies.element_mass
.species.mass
with a single float value was a bug. Then redefining is a bugfix, thus does not qualify as a breaking change.