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

Fix lobsterin dict inheritance and treat \t in lobsterins correctly #3439

Merged
merged 16 commits into from
Nov 2, 2023

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Nov 1, 2023

Summary

Closes #3437 and #3438.
Inheritance of the dict was not carried out correctly. I have started here with inheriting from UserDict. We will add more tests to avoid similar issues in the future.

@JaGeo
Copy link
Member Author

JaGeo commented Nov 1, 2023

@naik-aakash , I have started with the fix. I will test methods like update, delete as well.

We could also directly implement a fix for reading lobsterins with \t

@naik-aakash
Copy link
Contributor

Thanks! Thats great. So you will also fix the reading lobsterins with \t issue in this PR itself?

@JaGeo
Copy link
Member Author

JaGeo commented Nov 1, 2023

@naik-aakash You are invited to contribute to this PR. I am in the train today and I don't really expect to be able to work much on the remote connection 😅

@naik-aakash
Copy link
Contributor

@naik-aakash You are invited to contribute to this PR. I am in the train today and I don't really expect to be able to work much on the remote connection 😅

Ah okay. Just got the invitation. Thank you 😄

@JaGeo
Copy link
Member Author

JaGeo commented Nov 1, 2023

I think I have now correctly implemented the inheritance from a UserDict. All standard functionalities work.

Things left to do:

  • clarify how lobster reads cohpsteps (int, float): i.e., should we include a new category of lobster arguments?
  • treat \t in lobsterins incluing whether Lobster can process them and test every different functionality

@naik-aakash
Copy link
Contributor

naik-aakash commented Nov 1, 2023

I think treating \t is solved as well, have updated the existing test file to check the implementation and it seems to work fine .

I also tested if LOBSTER is able to read lobsterin files with \t as seperator between keyword and value (also at end of line). It does work fine

@JaGeo
Copy link
Member Author

JaGeo commented Nov 2, 2023

Update: LOBSTER can read cohpsteps as a float and integer. It will automatically round to the next larger integer independent on whether it is a float or an integer (in versions 4.1.0 and 5.0.0). I think it doesn't really matter that we print a float... Also, I think this behavior of LOBSTER is a bit unexpected (instead of 308, Lobster uses 309).

@JaGeo JaGeo changed the title WIP: Fix lobsterin dict inheritance Fix lobsterin dict inheritance Nov 2, 2023
@JaGeo
Copy link
Member Author

JaGeo commented Nov 2, 2023

@janosh , I think this would be ready for review in case you have time.

@JaGeo JaGeo changed the title Fix lobsterin dict inheritance Fix lobsterin dict inheritance and treat \t in lobsterins correctly Nov 2, 2023
@janosh janosh added the io Input/output functionality label Nov 2, 2023
@janosh janosh added fix Bug fix PRs lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction) labels Nov 2, 2023
@janosh janosh merged commit 28c8ebb into materialsproject:master Nov 2, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It seems that there is a bug to write the lobsterin file
3 participants