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

Notes from first time usage and APIs #17

Open
bocklund opened this issue May 8, 2024 · 3 comments
Open

Notes from first time usage and APIs #17

bocklund opened this issue May 8, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@bocklund
Copy link
Member

bocklund commented May 8, 2024

calculate_KS2022_randomSolutions always prints all input in parallel mode

print(pairedInputAndSettings)

It may be kind of noisy to always print the input without being behind a verbose flag if the compList is very long (especially since it's a lot of pymatgen Structure objects). Feel free to close

@amkrajewski
Copy link
Contributor

It's a leftover from some debugging. Already fixing it!

@bocklund bocklund changed the title calculate_KS2022_randomSolutions always prints all input in parallel mode Notes from first time usage and APIs May 8, 2024
@bocklund
Copy link
Member Author

bocklund commented May 8, 2024

I have some more general notes, and might just leave them here. Feel free to convert any to issues or I can elaborate if needed.

  1. It's surprising that SIPFENN mutates input. For example, calling Calculator.calculate_KS2022_randomSolutions with compList=composition_strs where composition_strs is some list of strings that I created, calculate_KS2022_randomSolutions will mutate the composition_strs list that I passed in and change it's type from List[str] to List[Composition].
  2. Calculator.loadModelCustom seems to require a separately specified model directory and network name here. Maybe it would make more sense to take in a single argument for the path to a network file? If you want to support local directory with paths, I think that should be possible too. To be extra, you could take in a list of paths to search for NNs by name (similar to the PATH variable used by operating systems)
  3. related, it might be a nice convenience if the modelNamein loadModelCustom was simply the networkName by default. At least for some of the methods I've been using, like get_resultDicts() it doesn't seem like the name I provided is used anyway.
  4. It seems inconsistent that Calculator.loadModelCustom hardcodes a onnx extension to the file, but other places like writeDescriptorsToCSV documents that "The file must have a '.csv' extension to be recognized correctly", but this actually isn't a requirement of the code (and isn't enforced anyway) and seems to be more of a suggestion if users want to use the CSV with something that depends on file extensions to recognize the type. I would err on the side of having users include full filenames with extensions, and only using extensions to the extent that they are hints about the type of file being read.
  5. (more of a bug, but I'll leave it here) Calculator.get_resultDictsWithNames fails without a nice warning or error if the inputNames member isn't set on Calculator instances

@amkrajewski
Copy link
Contributor

@bocklund Thank you for taking your time and providing great feedback! As always 😃

  • 1 - The idea was that the user's input would be expanded internally, and the exact thing it was run on would be made available that way. The same thing happens for baseStructList. I can see, of course, how this could cause issues. I plan to trim down this functionality into string representation of each structure-composition pair and move that into the inputFiles to address 5 with one stone.
  • 2 - In the older (pre-2021) versions, that's exactly how it worked by scanning through a list of paths and harvesting applicable models. I liked that, but I found out most users generally preferred very different names from me (SIPFENN_Krajewski2020 Novel Materials Model vs SIPFENN_Krajewski2020_NN20_B2048_E520_FP16_SimpNorm) hence, automatic handling of the default models from a fixed location and manual loading of models for advanced users. It could certainly be expanded if time allows.
  • 3 the get_resultDicts was intended to print the user-friendly modelName. Thanks for catching that!
  • 4 - Thanks for pointing that out! loadModelCustom will be adjusted so that the path passes directly if it has onnx extension, error gently if it has another extension, and append it only on extensionless paths.

@amkrajewski amkrajewski self-assigned this Aug 23, 2024
@amkrajewski amkrajewski added the enhancement New feature or request label Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants