Skip to content

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Aug 14, 2025

@atsju atsju requested review from githubdoe and gr5 August 14, 2025 16:29
Copy link
Collaborator

@gr5 gr5 left a comment

Choose a reason for hiding this comment

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

builds and runs fine

@atsju
Copy link
Collaborator Author

atsju commented Aug 17, 2025

@githubdoe can you have a look at this small pull request ?
All I did is moved zapm.cpp around to keep track it's an external file and add licence file.


Additional question:
Out of curiosity I checked if there had been some update on Michael side and it looks like there are small changes from original file.
image
I think you did that on purpose as I didn't find any historical file from Michael matching this. Can you please confirm if this is intentional ?
Maybe I could add some comments about it "Dale modified for " so that anybody looking into this in 10 years from now doesn't get too much headache :)

@githubdoe
Copy link
Owner

You can delete it. I don't need to see it anymore.

@githubdoe
Copy link
Owner

I don't remember those changes. Sorry..

@githubdoe
Copy link
Owner

If I understand perhaps Mike made changes that I did not pick up. I'm not sure which file is mine and is Mike's in you example.

@atsju
Copy link
Collaborator Author

atsju commented Aug 17, 2025

Oh I see now! You copied zapm to zernikeprocess.cpp. thank you

If I understand perhaps Mike made changes that I did not pick up. I'm not sure which file is mine and is Mike's in you example.
no I checked and that's not the case. https://github.com/mlpeck/zernike/blob/master/src/zapm.cpp I looked at history and Mikes version is like this since the beginning.

Let me close this pull request. I will look at zernike zapm.cpp file deletion. I might put zapm in a separate file just for licence tracking purpose.

@atsju atsju closed this Aug 17, 2025
@atsju
Copy link
Collaborator Author

atsju commented Aug 17, 2025

green is mike. Red is yours

@gr5
Copy link
Collaborator

gr5 commented Aug 17, 2025

I use slashes below ("/") to indicate synonymous terms. Super valuable information follows to understand DFTF zernike stuff:

The difference you highlighted above are the normalizers aka m_norms[] in dftf. For example for S.A. the norm is sqrt(5). There's one for each zernike polynomial. They are scalars/constants.

There are 2 columns in dftf that show the zernike coefficients: wyant, rms. The difference is a multiplier called the normalizer.

The basic process to take a waveform and find the zernike coefficients is you create a waveform for each zernike. e.g. if there are 49 then you create 49 waves/shapes/arrays/polynomials. One for each zernike polynomial. Then you find the best fit of each one to get the coefficients. The coefficients are what appears in the dftf gui.

To go backwards you can multiply each of the 49 coefficients by each of the 49 polynomial shapes to get the original wavefront (but with only 49 polynomials you lose a lot of detail for a 100x100 pixel wavefront you need 10,000 zernikes to lose no detail).

Back to the normalizers: on the zernike polynomial wikipedia page, some of the polynomials include the normalizers and some don't.

If you include the normalizers in your starting polynomials you will get the RMS column for your coefficients.
If you don't include the normalizers in your starting polynomials you will get the Wyant column.

You can quickly convert one column to the other by multiplying or dividing by the normalizers (m_norms[]).

I believe that Michael Peck includes the normalizers when he calculates the zernike polynomials and ends up with the RMS column and Dale I believe doesn't use them and ends up with the Wyant column data. Again you can then just use m_norms to get the other column of data. That's why the code is different.

There's a small possibility I got the columns backwards but the point is the normalizers can be included in the polynomial waves to get one column or not included to get the other column and then you can convert one column to the other by multiplying or dividing by the normalizers.

@atsju atsju reopened this Aug 17, 2025
@atsju atsju closed this Aug 17, 2025
Comment on lines +1274 to +1276
// zpmCxx commented out, not used, might be needed in doPSIstep4
// I (atsju) recommend deleting this part and add original or modified file from Mike when/if needed
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you agree I should remove zpmCxx or I let it commented like this ?


arma::mat rhotheta( int width, double radius, double cx, double cy, const wavefront *wf = 0);

arma::mat zpmC(arma::rowvec rho, arma::rowvec theta, int maxorder);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is what I propose to put into zernike/zpm.cpp

@atsju
Copy link
Collaborator Author

atsju commented Aug 17, 2025

Thank you this actually makes more sense now. I did not delete zapm.cpp as proposed by @githubdoe as it's actually used.
What I did instead is remove the zapmC (old name of Mike zapm) from DFTFringe code as it was not used. I did some cleanup but the code that runs is the same as before.
So this PR grow a little bit because of that but source is same.

What I propose in a different PR to make everything even is to remove zpmC (old name of zpm) and add Mike zpm.cpp as it is done for zapm.cpp with little commented modifications.
This would make it easier to follow I suppose and keep track of authors. I checked code and their are identical so it's more like moving things arround.

@atsju atsju reopened this Aug 17, 2025
@githubdoe
Copy link
Owner

githubdoe commented Aug 17, 2025

Thank you George for remembering the Normalizing terms. Yes that is what makes mine different. The zpm and zpmC were related and I have a memory of the zpmC being reserved for future use but that may be a false memory. I use to remember what the "C" stood for. Perhaps that will came back to me. So now I'm unsure that it should be deleted. But if I don't remember it's purpose and if George does not as well then it probably does not matter. Oh I think maybe the "C" stood for circular......But....

Yes I think it was reserved to be used in the more advanced PSI process that never got implemented. Mike has since modified his PSI algorithms I think which also may make zpmC obsolete.

@atsju
Copy link
Collaborator Author

atsju commented Aug 17, 2025

Dale I checked and it has just been renamed in latest releases. ZapmC became zapm and zpmC became zpm. Those are same. Maybe the C stands just for the language C. As Mike does lot of R this would make sense. Maybe ask him.

What you might remember however was that you implemented both zpm(C) and zapm(C) where "a" probably stands for annular. And you copied the new zapm instead of using your existing zampC (which are the same - remember renaming).

I think this could indeed make sense with your partial memories.

@githubdoe
Copy link
Owner

Good work. Yes you got the naming right and I do remember it now. Yes the a was for annular. C was probably Mike's naming convention. I am glad you figured that all out. I'm too far removed from that code these days to instanly recover those things.

So now zapm.cpp is redundent because I/George integrated it into you code. Yes?

@atsju
Copy link
Collaborator Author

atsju commented Aug 18, 2025

Yes zapm.cpp was integrated redundantly with the function zernikeprocess::zapmC.
I removed the later because it was not used for compilation before my change. And keep zapm.cpp because it was used.
I think this is the way to go because:

  • it is what was compiled so it is also the most tested version
  • it keeps Mike's things in his own files (even if slightly modified)

@atsju atsju merged commit fc352c4 into githubdoe:master Aug 20, 2025
28 checks passed
@atsju atsju deleted the JST/moveZernike branch December 23, 2025 14:03
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.

3 participants