Skip to content

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

Merged
janosh merged 16 commits intomaterialsproject:masterfrom
JaGeo:fix_lobsterin
Nov 2, 2023
Merged

Fix lobsterin dict inheritance and treat \t in lobsterins correctly#3439
janosh merged 16 commits intomaterialsproject:masterfrom
JaGeo:fix_lobsterin

Conversation

@JaGeo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@JaGeo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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