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

deprecate things that are not being done properly #44

Closed
bernstei opened this issue Mar 22, 2022 · 12 comments · Fixed by #74
Closed

deprecate things that are not being done properly #44

bernstei opened this issue Mar 22, 2022 · 12 comments · Fixed by #74
Labels
deprecation this functionality or implementation should be deprecated or removed short term minimal work expected

Comments

@bernstei
Copy link
Contributor

Deprecate bits of code that don't do things the workflow way, or just don't use it. They can be updated to do it the right way (so they can be wrapped by iterable_loop (soon to be autoparallelize), or moved to some user-specific place.

reactions_processing/
plotting/
some things in generate, e.g.
neb, ts, vib, radicals

some things in cli.py

@bernstei bernstei added short term minimal work expected deprecation this functionality or implementation should be deprecated or removed labels Mar 22, 2022
@hheenen
Copy link
Contributor

hheenen commented Apr 29, 2022

Considering the plotting/, I understand this is not linked to the autoparallelize, but I wonder if you don't want to provide some standard plots (one may add more) which could help users to monitor progress of potential training?

If this is not wanted in the wfl/ code maybe it makes sense to move it out of it, but keep it separately in the repo?

@bernstei
Copy link
Contributor Author

bernstei commented Apr 29, 2022

I don't even know what's in plotting/, but maybe we need a different repo for analysis?

@hheenen
Copy link
Contributor

hheenen commented Apr 29, 2022

I mean that's up to you and maybe a separate repo is easier to maintain. I personally think that providing some analysis tools for workflow in the same repo would be more convenient, though. So far some of us have been using functions like plot_ef_correlation.py producing detailed energy and force correlation plots. Of course we could generalize and refactor the contained functions and add some more of our own which could fit in there -- we also happily do this if you chose to make a separate analysis repo.

@bernstei
Copy link
Contributor Author

@gabor1 where should we put these things?

@gelzinyte
Copy link
Contributor

Just to collect some other things to revise:

  • devtools (in particular devtools/scripts/pretty_pca.py)
  • (to be continued)

@gelzinyte
Copy link
Contributor

@bernstei what's wrong with vib.py?

@bernstei
Copy link
Contributor Author

bernstei commented May 2, 2022

It's sort of OK, since it does use ConfigSet, but wouldn't it be more modular to have functions that just generate the necessary configurations, then use the existing autoparallelized generic eval loop, and then routines that analyze them? Given that the calculations will almost certainly dominate the cost that seems like a more useful packaging of the work.

@Zausinator
Copy link
Contributor

Very small nitpick, but might I suggest that all style elements be removed from the plotting section? Seems like that is something that the users can easily set via their plt.rcParams. So instead of defining a sequence of colors, we just define:

colors = plt.rcParams['axes.prop_cycle'].by_key()['color']

and iterate through those parameters. The figsizes, labelsizes, etc. would then all be taken directly from the user's individual setups.
The added benefit would be that the code becomes a lot shorter.

An alternative approach would be to provide a default rcParams file somewhere, which would then be the fallback.

@gabor1
Copy link
Contributor

gabor1 commented May 5, 2022

providing a default rcParams would be good. not everyone controls their styles by rcParams files

@bernstei
Copy link
Contributor Author

bernstei commented May 5, 2022

Comments on what's there now:

  • reactions_processing/ : routines specific to @stenczelt workflow, not really workflow ops [note - by this reasoning, should gap-rss-iter-fit be moved out of workflow as well, or at least refactored in things that belong a things that don't?]
  • plotting/ : nice for analysis, but not really using any workflow stuff
  • some things in generate/,
    • neb : not using any workflow features
    • ts : not using any workflow features
    • vib : using autoparallelize over configs xor Hessian displacements in each config.
    • radicals : not using any workflow features

I think that vib has a good reason to stay, but all the others should probably be moved outside of wfl, whether that's some user or system specific directory distributed with the workflow, or entirely separate. I've tentatively moved them to workflow/deprecated.

As for vib, we should think about the best interface, and perhaps also making it uniform with the equivalent functionality for periodic systems (using phonopy). I have routines for that, but would need to think about how to unify the two interfaces.

@bernstei
Copy link
Contributor Author

bernstei commented May 5, 2022

Current plan:

  • reactions_processing, neb, ts, radicals to user/reactions (name to be finalized)
  • plotting to deprecate/plotting, to be moved into a separate directory or repo for analysis
  • vib unchanged for now, pending thought about merging with phononpy based code for periodic systems.

@gabor1 sounds OK to you? Do we have a better name for the user directory for Tamas's code?

@stenczelt
Copy link
Member

stenczelt commented May 6, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation this functionality or implementation should be deprecated or removed short term minimal work expected
Projects
None yet
6 participants