Reconstruct slc from inteferograms #9
Conversation
|
Hi @FreekvanLeijen, this PR is ready for review. After merging #15, all the changed files now are relevant for your review. |
|
Hi @meiertgrootes, this is the PR I talked about. I implemented a function for reconstructing radar imgs from interferograms (conj multiplication of two radar images), and provided slurm + python example scripts. Can you give it a look? Thanks! |
|
|
||
| # ++++ 5 - prf | ||
| pattern = r"Pulse_Repetition_Frequency \(computed, Hz\):" + SC_N_PATTERN | ||
| match = re.search(pattern, content) |
There was a problem hiding this comment.
There is a huge number of the regular expression searches here and in the following. I'd suggest moving this to a separate module and making a wrapper which calls them by name, allowing the correct implementation to be retrieved. This also makes one central place for keeping track of all these things and easily supports adding additional methods as needed.
Happy to discuss @rogerkuou
There was a problem hiding this comment.
Hi @meiertgrootes, I like this idea! I added your suggestion as an item in issue #14. My thought is to build a static dict, mapping the attribute name and searching pattern. Let me know what you think.
|
Thanks @meiertgrootes for the review! I made changes according to your comments. Only for the suggestion on RE pattern in |
|
Hi @FreekvanLeijen, thanks a lot for the review! Just to check, do you have more comments? I am implementing |
| "imag": slc_recon_output["complex"].imag.astype(np.float16), | ||
| } | ||
| ) | ||
| slc_recon_output = slc_recon_output.drop_vars(["complex", "amplitude", "phase"]) |
There was a problem hiding this comment.
Question: so in sarxarray.from_binary we create these complex, amplitude and phase variables. And here we drop them again. In general, I do not think we should save these variables to .zarr files anyway (are we?). They can be computed on the fly from the real and image part, right?
So to consider: change sarxarray.from_binary to remove the amplitude, phase, complex. Can be done later on the fly.
There was a problem hiding this comment.
Hi Freek,
By using sarxarray.from_binary, the following happens to the three variables:
complexis what is originally saved in binary file, we create a task map instruct how to read it- for
phase/amplitude, we create a task map of how to derive them fromcomplex
Therfore, regarding your comments about compute amplitude, phase, and complex on the fly, actually it is already what we are doing. These three values are "lazy" variables, which means they are not actually computed until they are needed. In Line 133, what we drop here are simply task graphs of how to create them, so not redundant computation are involved. They will not be saved into Zarr.
In the current reslc implementation, we create task graphs of how to derive real and imag from complex. When writing the restored SLCs to the disk, what happened is the complex number will be read into the memory, sparated in to real and imag, and written to disk. The separation operation is not costy. Therefore comparing directly writing complex to disk, we almost have no efficiency loss.
By saying this, I think what we need is not to remove the three variable from sarxarray.from_binary, but to create a new function (I am thinking sarxarray.from_zarr) to directly create task graphs to get complex, amplitude and phase from real and imag fields.
| # Rechunk and write as zarr | ||
| slc_recon_output = slc_recon_output.chunk({"azimuth": writing_chunks[0], "range": writing_chunks[1]}) | ||
| if overwrite_zarr: | ||
| slc_recon.to_zarr("nl_veenweiden_s1_dsc_t037.zarr", mode="w") |
There was a problem hiding this comment.
Two things:
- I do not see the mother image being written to cpxfloat16. Can we add?
- How about the h2ph file. Add to this .zarr, or a separate one? To discuss.
There was a problem hiding this comment.
Hi Freek,
as we discussed on Tuesday, I have put these two action points in issue #22, since they need to be tested on Spider. I will work on them after finishing issue #10 (ps selection).
For now the script only output non-mother SLCs in np.float16, with real and imag part separate. Just to confirm, as we discussed on Tuesday, there is no cpxfloat16 available in numpy (I think you meant np.complex32? anyhow neither exists in numpy, see here the explaination) so we are going to store real and imag part separately, both in np.float16.
There was a problem hiding this comment.
Hi Ou,
Sorry, I should be more clear in my writing. With cpxfloat16 I meant the 'concept' of storing the complex data in float16 format, indeed the real and imaginary part separately. So as you described.
Move the two-way balance to line 61
|
Hi @FreekvanLeijen, thanks a lot for the review! I adapted most of your comments! There are several things I decided to put them into the todo lists, they are:
With this info, would you like to take another look and see if there are other things you want to add? Or some things you think should be fixed in this PR? |
|
Thanks @FreekvanLeijen for approval, will merge. |
fix two issues:
slc.pyexamples/scripts, with README file, example SLURM script, and example Python file.