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

Attaching user defined logfile to MD wrapper. #327

Closed
jungsdao opened this issue Jul 26, 2024 · 10 comments
Closed

Attaching user defined logfile to MD wrapper. #327

jungsdao opened this issue Jul 26, 2024 · 10 comments

Comments

@jungsdao
Copy link
Contributor

I'd like to attach user defined MD logger to my MD run.
When I use autoparallelize and initiate multiple MD jobs simultaneously, defining logfile name for respective process is not easy.
I can perhaps modify from output file name given into OutputSpec.
Is there any way to access to information in OutputSpec from MD so that I can generate logfile for individual processes?
Thanks in advance!

@bernstei
Copy link
Contributor

Turns out that that's a very subtle thing, and while I understand why you want it, to some extent it violates the assumptions that were designed into wfl.

I can think of two approaches

  • the cleaner one from the point of view of the design of wfl is not to log into a bunch of files, but just store the relevant info in the info dict of the returned Atoms objects
  • pass the wrapped MD a string that's used as the beginning of the filename for every config, and use low-level wfl functionality to add a per-config dependent suffix, e.g. the number of the config in the list of inputs. There's already something like that because every call needs a different random seed, so it wouldn't be a lot of work (for me :) - I doubt anyone else understands the low level stuff well enough to do it right now)

@jungsdao
Copy link
Contributor Author

Currently I was postprocessing to calculate the properties that I want to log and I just wanted to make things easier by attaching logger to MD. Adding property to info is actually less preferred because just reading logfile is much faster than reading xyzfile.
However, I'm fine with postprocessing method as well.

@bernstei
Copy link
Contributor

I'll look over the existing code and see how hard it'd be to add.

@bernstei
Copy link
Contributor

I think it would be fairly easy to do, actually. Should I add an MDLogger_kwargs optional argument? It would add the number of the configuration to the logfile.

@bernstei
Copy link
Contributor

bernstei commented Jul 26, 2024

Do you want to try the attached patch? There are new logger_kwargs and logger_interval optional arguments that are used to construct and attach the logger. You at least need include the kwarg logfile (string, not open file) that it will add the item # to.

mdlogger.patch

@bernstei
Copy link
Contributor

@jungsdao do you want to check this patch?

@jungsdao
Copy link
Contributor Author

jungsdao commented Jul 30, 2024

Sorry for belated reply, I just have tested and it works pretty fine.
Thank you for your help!
However, I think it should allow accepting user defined MDLogger derived object other than original MDLogger.

107     if logger_kwargs is not None:
108         logger = logger_kwargs.pop("logger", MDLogger)
109         logger_logfile = logger_kwargs["logfile"]

It can be added like this.
Also not related to this issue but I think MD wrapper should log current temperature as well, not just temperature assigned in the initialization. I think this can be easily added to info dict by adding following line to process_step

at.info["MD_current_temperature"] = at.get_temperature()

Maybe I could add a PR about this.

@bernstei
Copy link
Contributor

Sorry for belated reply, I just have tested and it works pretty fine. Thank you for your help! However, I think it should allow accepting user defined MDLogger derived object other than original MDLogger.

107     if logger_kwargs is not None:
108         logger = logger_kwargs.pop("logger", MDLogger)
109         logger_logfile = logger_kwargs["logfile"]

It can be added like this.

You'd also need to modify (around line 253)

  logger = MDLogger(**logger_kwargs)

to

  logger = logger(**logger_kwargs)

Also not related to this issue but I think MD wrapper should log current temperature as well, not just temperature assigned in the initialization. I think this can be easily added to info dict by adding following line to process_step

at.info["MD_current_temperature"] = at.get_temperature()

We could, but keep in mind that the returned trajectories should have momenta saved, so you can get the temperature after the fact.

Maybe I could add a PR about this.

Feel free to do that with this patch.

@bernstei
Copy link
Contributor

Actually, you'll need to be a bit more careful with the logger variable, because the construction (like ~253) needs to happen multiple times. You'll need to save it (pop from the kwargs) as something like logger_constructor, and then around like 253 do logger = logger_constructor(**logger_kwargs).

@bernstei
Copy link
Contributor

closed by #328

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

No branches or pull requests

2 participants