Skip to content

loading the datetime of the analysis into args #64

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

Closed
wants to merge 1 commit into from

Conversation

yucongalicechen
Copy link
Collaborator

  • Initial commit, use mocking datetime for testing.
  • In labpdfprocapp.py, I loaded the time before looping and correcting each input file. Not sure if this is the best, but I think it should be loaded before we create input_pattern DO, so that later we can load it with all other metadata into the other DOs using metadata=vars(args).
  • The function returns datetime to the accuracy of microseconds (i.e., an example of datetime.now() is 2024-05-20 10:50:27.76998). I only put accuracy to seconds in the test because it seems neater. Not sure what level of accuracy would we want?

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loading the datetime is just two lines of code, so I think we want to do this in the function in the code where we do all the things and not have its own public function. So just write tests for the control function that applies the correction, and this is part of the workflow of applying a correction.

Also, I thought the discussion was to use freezetime rather than mocking it? I don't really mind either way, with a slight preference for whichever is easier to read and maintain. i.e., the one that is more intuitive.

@sbillinge
Copy link
Contributor

thinking more, maybe we put this in apply_corr() but we use a pattern that we add the datetime to the diffraction object metadata. We need to check somewhere at the end that all metadata has been successfully dumped to the file, which is the main outcome we want. Maybe we just want to test that? So after writing the file we have a test that reads the file and make sure everything we expect is there? It will be abit hard to maintain because we will have to change this test every time we add anything, but it is actually testing the thing we really care about....

@yucongalicechen
Copy link
Collaborator Author

If we want to add the time stamp in the functions, shall we do it in compute_cve(), as we also need the time in the absorption correction DO? I think it is okay and probably more straightforward that we add the time stamp manually into the metadata and test for it.

Regarding the freezegun, I’m having a weird runtime error today importing it. It’s probably something on my end. On the other hand I’m thinking that since I’ll use mocking for testing username and email, it’s probably better that we maintain the same style?

@sbillinge
Copy link
Contributor

sbillinge commented May 21, 2024 via email

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a bit more and I think we need a apply_correction() function that takes a list of DOs and returns a list of corrected DOs.

The test for this would be that the expected and actual diffraction objects are identical if we can think of a way of doing that. Then we have the right time-stamp in the expected etc.

@sbillinge
Copy link
Contributor

The best way to assert that instances of the DO are equal is actually to write an eq function in the DO itself..... Please could you put a PR into diffpy.utils with that? e.g., see here:
https://stackoverflow.com/questions/1227121/compare-object-instances-for-equality-by-their-attributes

@yucongalicechen
Copy link
Collaborator Author

  • I'll use monkeypatch for other functions. I think I'm getting some errors using freezegun so maybe I'll keep the mocking datetime for now.
  • In compute_cve we will also have a DO that will be written into the cve output file.. Were you suggesting that we use apply_correction() before we return a DO each time?
  • I'll make a PR to diffpy.utils

@sbillinge
Copy link
Contributor

  • I'll use monkeypatch for other functions. I think I'm getting some errors using freezegun so maybe I'll keep the mocking datetime for now.

    • In compute_cve we will also have a DO that will be written into the cve output file.. Were you suggesting that we use apply_correction() before we return a DO each time?

    • I'll make a PR to diffpy.utils

Sorry, I am not sure I understand your question on the second bullet, but the basic idea with the date-time is that it records in the file the time when the file was created, basically, so we could record a time in the cve file when the cve file is created if it is requested by the user but for the <data-name>_corrected.chi files (that contain corrected data) we would like the datetime where that data were corrected.

We are using DO.dump() to write the file I think. I don't remember how I wrote that, but I think it writes a bunch of stuff in the header of the chi file that it knows about. We can tweak how it does that if it makes sense, but mostly I think anything that is in the DO metadata will get written there, so we load the dict(args) and anything else we want (like creation_time) in there and it will get written.

Does that make sense?

@sbillinge
Copy link
Contributor

closing this as not needed. Now the time-stamp is added in the file dump from the diffraction_object.

@sbillinge sbillinge closed this Jun 6, 2024
@yucongalicechen yucongalicechen deleted the datetime branch June 6, 2024 22:13
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

Successfully merging this pull request may close these issues.

2 participants