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

replace full output info in __str__ by chemical formula #439

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Conversation

samwaseda
Copy link
Member

In order to exploit the feature here, I would like to suggest to largely simplify __str__ in Atoms by replace the full info by the chemical formula. this allows us to set something like:

job = pr.create.job.Lammps(('lammps', structure))

@coveralls
Copy link

coveralls commented Nov 25, 2021

Pull Request Test Coverage Report for Build 1516586052

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 63 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.05%) to 69.855%

Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/structure/atoms.py 1 74.67%
pyiron_atomistics/atomistics/structure/neighbors.py 4 96.38%
pyiron_atomistics/atomistics/job/structurecontainer.py 8 75.61%
pyiron_atomistics/atomistics/job/atomistic.py 50 72.87%
Totals Coverage Status
Change from base Build 1500854841: 0.05%
Covered Lines: 11450
Relevant Lines: 16391

💛 - Coveralls

Copy link
Member

@niklassiemer niklassiemer left a comment

Choose a reason for hiding this comment

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

I like this solution. In my notebook testing nothing changes on just providing a structure, since the notebook ignores __str__ and uses __repr__. Just if you print(structure) you just get the chemical formula.

@pmrv
Copy link
Contributor

pmrv commented Nov 25, 2021

I see how this is useful, but I also think it's nice to have just the text dump of a structure, especially when checking small structures.

@samwaseda
Copy link
Member Author

I see how this is useful, but I also think it's nice to have just the text dump of a structure, especially when checking small structures.

Is there a case that __str__ is used and not __repr__?

@niklassiemer
Copy link
Member

On print or str() 😅

@liamhuber
Copy link
Member

liamhuber commented Nov 25, 2021

Maybe we replace __str__ with the formula as suggested, but add a to_str() method (or similarly named) that wraps the string rep in __repr__

@samwaseda
Copy link
Member Author

But I still don't understand what would make you use print or str. If that's really the case, I would rather support @liamhuber 's idea of adding to_str, but frankly I'd personally not know when I'd ever use it.

@niklassiemer
Copy link
Member

I am not sure if we would need this. With this change we would get after all:

Fe = pr.create.structure.bulk('Fe')
Fe
>>> Fe: [0. 0. 0.]
>>> pbc: [ True  True  True]
>>> cell: 
>>> Cell([[-1.435, 1.435, 1.435], [1.435, -1.435, 1.435], [1.435, 1.435, -1.435]])
# same as `print(repr(Fe))`

only

print(Fe)
>>> Fe
str(Fe)
>>> 'Fe'

would change. The only reason for a to_str() method would be if we would need the long string output for something else than to display for human readability. I.e. structure_str = stucture.to_str() and I just do not see any case where I would need to get a string Fe: [0. 0. 0.]\npbc: [...].

@liamhuber
Copy link
Member

Yes, it's exactly as you say, Niklas. I am also not immediately sure what the application might be for needing the full string, but I share Marvin's reticence for getting rid of it entirely. It clutters the namespace a little bit, but I'd be more comfortable if we kept the string accessible by a special method. Otherwise installs like the change in the default behaviour

@niklassiemer
Copy link
Member

I am ok with adding that method, however, if I really needed that string I could just do structure_as_string = repr(Fe)?

@samwaseda
Copy link
Member Author

If we don't know whether to_str could be useful or not, then I'd rather not add it in the first place, because Atoms has in my opinion largely surpassed the number of options that can be seen in the auto complete. So whoever for some reason happens to need the string version of __repr__ will most probably simply take repr(structure) (as @niklassiemer suggests) and not look for the extra function that may or may not exist and may or may not do what the user wants it to do.

@samwaseda
Copy link
Member Author

Following today's pyiron meeting, I'll merge this one as soon as the tests pass.

@samwaseda samwaseda merged commit 2156964 into master Nov 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the atoms_str branch November 29, 2021 16:36
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.

None yet

5 participants