spacetelescope / PASTIS

Algorithm for analytical contrast predictions of coronagraphs on segmented telescopes
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Save efields in form of fits cubes at the WFS plane and Science plane #130

Closed asahooexo closed 2 years ago

asahooexo commented 2 years ago

This PR adds extra functions to create sensitivity matrices required for WFS&C analysis using PASTIS tolerances. Sensitivity matrices comprises a list of electric fields which are obtained by poking the deformable mirror with one modal aberration at a time

This PR only saves electric field at the WFS plane and Sciene plane. Sensitvity matrices are essentially stacked electric fields. These fields (both at focal and wfs plane) are calculated by poking the DM by one aberration mode and subtracting the reference unaberrated electric field.

For simplicity, this PR only saves the poked efields. Subtracting the reference electric field and reshaping will be done elsewhere, and is outside this PR

ivalaginja commented 2 years ago

Hey @ananyasahoo1904, could we rename calculate_sensitivity_matrix_from_efields() to calculate_wfs_matrix_from_efields() or the like? Those are sensitivity matrices in all of the planes (science plane and WFS plane) so this would make it easier to distinguish.

asahooexo commented 2 years ago

@ivalaginja Yes, it would be easier to distinguish, won't there be too many methods?

ivalaginja commented 2 years ago

@ivalaginja Yes, it would be easier to distinguish, won't there be too many methods?

I am not quite sure I understand the question, my apologies, could you maybe rephrase?

As far as I can tell, there are only two methods that calculate a sensitivity matrix, one in the science plane (calculate_pastis_matrix_from_efields()) and one in the WFS plane. If we ever start making more we could consolidate for sure, but I don't see a reason for that right now.

I also just saw that calculate_sensitivity_matrix_from_efields() currently calculates both matrices, which makes the flag self.calc_science obsolete, and we don't want that. So I think this method can be thinned down to just calculating the sensitivity matrix on the WFS.

asahooexo commented 2 years ago

Okay, I will look into this and see if I can thin down. Previously, I thought bifurcating calculate_sensitivity_matrix_from_efields() into two pieces adds an extra function.

Currently, I am running into some issue of dimensional mismatch between notebooks and the repo, this breaks close loop analysis. The function calculate_sensitivity_matrix_from_efields() is fully not ready yet.

asahooexo commented 2 years ago

@ivalaginja this is ready to merge. Let me know if you any comments.

asahooexo commented 2 years ago

@ivalaginja can you please point at which line sensitivity matrix at the science plane (G_coron) is already calculated? It should be 3d array

ivalaginja commented 2 years ago

The sole purpose of these classes is to calculate the sensitivity matrix in the science plane. In particular, this happens in this line: https://github.com/spacetelescope/PASTIS/blob/9590bb36e253d23430435cdfc10b8083e540b9b5/pastis/matrix_generation/matrix_from_efields.py#L74

If we need this matrix in a different format moving forward, then we should connect over how to deal with reformatting rather than duplicate code. What's the third dimension in your implementation?

asahooexo commented 2 years ago

PASTIS matrix is not equal to sensitvity matrix in science plane. It comprises of two cubes of efields (imag and real) at focal plane. It is dimension = [totalpixels, 2, total number of modes] 2 in the dimension stands for real and imag for poked efields,

L74 clearly calculates a 2D PASTIS matrix, where each element is a scalar contrast.

Sensitvity matrices are used only for temporal analysis so lets generate them(both at science and wfs plane) together as a separate function "calculate_sensitivity_matrix_from_efields"

asahooexo commented 2 years ago

when changing the simulator, the efields size may vary, hence subsampling factor might vary, we need to change this in the config file.

ivalaginja commented 2 years ago

@ananyasahoo1904 let's not add analysis scripts in this PR, that's gonna lead to scope-creep. Shall we schedule a call on Monday to work on this and get it wrapped up?

asahooexo commented 2 years ago

@ananyasahoo1904 let's not add analysis scripts in this PR, that's gonna lead to scope-creep. Shall we schedule a call on Monday to work on this and get it wrapped up?

Monday works fine for me

ivalaginja commented 2 years ago

After an offline conversation with @ananyasahoo1904, we concluded the path forward was to get rid of the currently new function calculate_sensitivity_matrix_from_efields() and instead incorporate its different parts in appropriate places in the code:

The subsampling of the E-fields as well as their formatting needed for the temporal analysis will be handled over in the analysis section once we get to that implementation.

And just to repeat one requirement from further above:

ehpor commented 2 years ago

The regression test failure is due to the release of HCIPy 0.5.0. I ran the tests locally with both HCIPy 0.4.0 (passes) and HCIPy 0.5.0 (fails) to confirm. I'm not sure what the root cause of this is. The differences, shown below, are actually quite substantial, way higher than expected.

image

To avoid bogging PASTIS down, you might wanna pin HCIPy to version 0.4.0 in the environment.yml file for the time being.

asahooexo commented 2 years ago

@ivalaginja sensitivity matrices are not exactly PASTIS matrices, simply these are bunch of stacked electric fields. These cubes are calculated in this scripts and saved. We feed these saved matrices (of course reshaping, subsampling to 1d array type) to the close loop scripts. Remember we got rid of the function calculate_sensitivity_matrix_from_efields()?

I have added a new function that reshapes these matrices to be used in close loop scripts. Look PR #138

asahooexo commented 2 years ago

@ivalaginja I am going to change the title of the PR to "Compute and Save efields at the focal and WFS plane " Let me know if this sounds good to you.

ivalaginja commented 2 years ago

@ivalaginja I am going to change the title of the PR to "Compute and Save efields at the focal and WFS plane " Let me know if this sounds good to you.

Only WFS plane, since this already existed for the science plane before this PR.

ivalaginja commented 2 years ago

@ivalaginja sensitivity matrices are not exactly PASTIS matrices, simply these are bunch of stacked electric fields. These cubes are calculated in this scripts and saved. We feed these saved matrices (of course reshaping, subsampling to 1d array type) to the close loop scripts. Remember we got rid of the function calculate_sensitivity_matrix_from_efields()?

I have added a new function that reshapes these matrices to be used in close loop scripts. Look PR #138

We got rid of that function because it was doing too much at once, not because we didn't want these matrices to be calculated. But if I remember correctly, it also had something to do with the fact that the WFS matrix needed to be resampled on the go? Is that right?

asahooexo commented 2 years ago

@ivalaginja We got rid of the calculate_sensitivity_matrix_from_efields() because:

  1. This function was stacking and reshaping the calculated efields. No, new arithmetic.
  2. In past was I saving the sensitivity matrix (same as bunch of stacked efields) directly to 1d array format, which was unreadable, unable to open in ds9
  3. Now the efields is saved in 2D cube format, which can be easily opened and used by the close loop.
  4. I was subsampling only the WFS efields, to run the close loop analysis scripts faster. It is not necessary to be on the go.

In Conclusion, All I need this script is to save efields, and if possible in a cube format.

asahooexo commented 2 years ago

@ivalaginja I am going to change the title of the PR to "Compute and Save efields at the focal and WFS plane " Let me know if this sounds good to you.

Only WFS plane, since this already existed for the science plane before this PR.

Earlier, the focal plane science images were saved as individual fits files (separate imag and real part). Now I save them in a cube.

ivalaginja commented 2 years ago

I think we are blurring the lines between two distinct things here: one is whether something is done, the other one is how exactly it is done, which is important to define the scope of a unit of work. The science-plane data was being calculated way before this PR so in the worst case, it could always be reformatted wherever it is used. Of course it makes sense to standardize the outputs, what happened in this PR. The WFS-plane data on the other hand didn't even get calculated prior to this PR so we are fixing the whether - while also agreeing on the how. I think some of the misunderstandings might have come from mixing this up so I just wanted to stress it here. I was also having a hard time following all this because I was on and off, so that certainly didn't help.

I think we are good in not calculating the WFS-sensitivity matrix here as that is a little more involved than for the science-plane matrix, thanks for the clarifications above. In this case, I will just finish my tests and then do a (hopefully) final review.