Skip to content
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

Return type of atomic_symbol changed #134

Closed
rashidrafeek opened this issue Dec 7, 2024 · 3 comments
Closed

Return type of atomic_symbol changed #134

rashidrafeek opened this issue Dec 7, 2024 · 3 comments

Comments

@rashidrafeek
Copy link
Contributor

In the 0.5 update, it seems that atomic_symbol now returns a ChemicalSpecies object and not a Symbol.

using Unitful, AtomsBase
box = ([1, 0, 0]u"m", [0, 1, 0]u"m", [0, 0, 1]u"m")
pbcs = (true, true, false)
atoms = Atom[ChemicalSpecies(:C; atom_name=:MyC) => [0.25, 0.25, 0.25]u"m",
               :C12 => [0.75, 0.75, 0.75]u"m"]
system = FastSystem(atoms, box, pbcs)

typeof(atomic_symbol(system, 1))

returns ChemicalSpecies. Was this intentional? I was thinking that the atomic_symbol is supposed to return a Symbol, although the interface docs is also not clear about this. Currently there is a species function which returns a ChemicalSpecies object. So if this was intentional, these two functions seems to have the same functionality. Also, It might be helpful to clarify the return type in the interface docs.

@mfherbst
Copy link
Member

Yes indeed. I also noticed this, but I thought it was intentional. As mentioned by @cortner in #110 the problem with symbols is that they are not isbits, but ChemicalSpecies is. Perhaps this change was a little too quick as now we have a half-baked thing where some functions atomic_symbol return a ChemicalSpecies and others like element_symbol still return a Symbol. (Just to be clear: I can also see reasons for keeping it like this, but we should probably have a discussion about this).

At the very least I agree @rashidrafeek that it should be documented 👍. Are you able to attend the monthly call next Monday or is this too horrible wrt. your timezone ?

@rashidrafeek
Copy link
Contributor Author

Yeah, it would be great to clarify this in the documentation. Thank you for pointing out the earlier issue—I completely missed it. This seems to be a duplicate and can be addressed there, so I’ll go ahead and close this issue.

I’ll do my best to attend the monthly call on Monday. Will the timings be shared in the Zulip channel? If so, I can plan accordingly. Thanks for the invitation.

@mfherbst
Copy link
Member

They should be on the zulip, channel #monthly-calls. Times are published in the Julia Community Calendar

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

No branches or pull requests

2 participants