-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improve reslc example scripts #25
base: main
Are you sure you want to change the base?
Conversation
1. add mother image 2. add h2ph 3. add time coords 4. add lat and lon
Hi @FreekvanLeijen, could you please review this exmaple? In this PR I added the h2ph and mother slc to the output of the reslc process. Besides, the temporal info and the lat/lon are also added. There is an successfully executed example in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ou, looks nice. Two small requests from my side.
Hi @FreekvanLeijen, thanks for the review! I applied the comments you gave. Can you check again and see if you have further comments? |
Hi @FreekvanLeijen, regarding your comment of the precision loss when saving And I will also add @SarahAlidoost and @fnattino here to see if they also have comments on this. To summary the problem to all, this PR regards adding an example script for PyDePSI. In the end of script, we are saving an 3D array @FreekvanLeijen unfortunately I think your proposal of multiplying a factor does not work. I found this explaination roughly explain why it's not. However, considering the Attached is an visualizarion of the differences of h2ph between float32 and float16: I am not sure if this precision loss of 1e-10 is acceptable. If yes, then there are still two drawbacks of this solution:
On the other hand, for the same 2000x4000x409 slc stack, saving h2ph in float32 caused an extra 3GB storage. If we scale it to the entire stack, I expect a extra 300~400 GB.
With this info, at present, I would still recommend saving Attaching the notebook I used to run the expriment here in case you would like to check details: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogerkuou going through the code of the example I noticed two things:
- packages datetime (from datetime import datetime), dask.array (as da) and xarray (as xr) are not imported
- I think slcs_output should be written to zarr instead of slcs_recon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add to imports:
from datetime import datetime
import xarray as xr
import dask.array as da
Co-authored-by: Simon-van-Diepen <[email protected]>
Hi @Simon-van-Diepen, thanks for the comment. The missing modules has been updated. Can you check again? |
Hi @rogerkuou , I tested the appending of the time dimension on a live stack, and got some unexpected behaviour. In summary:
Append thus does not check for duplicates, and simply appends everything. I am currently testing the behaviour of mode="w" in case the stack does exist. |
Hi @rogerkuou , mode="w" works but I found another bug: if you print the time axis of the resulting zarr the following shows:
2020-03-28 is the mother and now appears twice. Can you have a look at what causes this? |
@rogerkuou I found that replacing line 165 by
properly removes the duplicated mother image |
Hi @Simon-van-Diepen, thanks for the feedback! I took a deeper look and thought about this a bit more. My opinion is, we should try to invest a bit more on On the one hand, I am glad I would experiment if it will help if we only read the binary file (phase, h2ph) of the new image and mother image, but read the exsting images from Zarr. I would need a bit more time to investigate this, probably after I come back from holidat (Nov 11). I would keep this PR open for now. |
Hi @rogerkuou , I agree we should investigate if we can make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ou, one more small request. Rest is fine.
Fix #22
Added the following parts to reslc exmaple scripts: