CosmoStat / wf-psf

Data-driven wavefront-based PSF modelling framework.
MIT License
19 stars 9 forks source link

Rename SimPSFToolkit.py module #128

Closed nadamoukaddem closed 8 months ago

nadamoukaddem commented 8 months ago

solves #112 Changes Made:

nadamoukaddem commented 8 months ago

There are a few more changes to implement.

Is it better to make all changes in one commit, or to commit each file separately?

jeipollack commented 8 months ago

If you're performing exactly the same change for several files you can do it in one commit. If it's a different type of change then it should go in another commit.

Also, please be sure to review the Workflow which took effect especially when it comes to branch naming conventions (in particular for next time).

nadamoukaddem commented 8 months ago

Okay, thank you, Jennifer. I'll make sure to follow the branch naming conventions next time. The conventions include three cases: Feature, Bug, and Hotfix. For my case, it will be 'enhancement/issue-number/short-description'?

jeipollack commented 8 months ago

Okay, thank you, Jennifer. I'll make sure to follow the branch naming conventions next time. The conventions include three cases: Feature, Bug, and Hotfix. For my case, it will be 'enhancement/issue-number/short-description'?

No, the names must be from the three categories. Which one would it be then?

nadamoukaddem commented 8 months ago

Okay, thank you, Jennifer. I'll make sure to follow the branch naming conventions next time. The conventions include three cases: Feature, Bug, and Hotfix. For my case, it will be 'enhancement/issue-number/short-description'?

No, the names must be from the three categories. Which one would it be then?

It's not a new feature, so I would say it's more like a bug.

nadamoukaddem commented 8 months ago

There are a few more changes to implement.

Changes have been implemented.

jeipollack commented 8 months ago

Thanks for the update. Did you test with runs of training and metrics to ensure no breaking behaviour?

nadamoukaddem commented 8 months ago

Thanks for the update. Did you test with runs of training and metrics to ensure no breaking behaviour?

Yes, but there are tests that require a GPU and they are skipped.

jeipollack commented 8 months ago

Not unit tests. I meant validation tests. Launch a batch script and make sure everything runs with no errors.

nadamoukaddem commented 8 months ago

Not unit tests. I meant validation tests. Launch a batch script and make sure everything runs with no errors.

Okay, I will try testing it with Google Colab because Feynman is undergoing maintenance.

nadamoukaddem commented 8 months ago

Not unit tests. I meant validation tests. Launch a batch script and make sure everything runs with no errors.

I tested the code with a few epochs and got the expected output without any errors.

jeipollack commented 8 months ago

Can you share your validation reports?

nadamoukaddem commented 8 months ago

wf-psf_renameModule.log

jeipollack commented 8 months ago

You uploaded a single log file of a run. I want to make sure you understand what is meant by validation reports that should be provided. Can you explain more what this run is? What are the outputs you expected?

nadamoukaddem commented 8 months ago

You uploaded a single log file of a run. I want to make sure you understand what is meant by validation reports that should be provided. Can you explain more what this run is? What are the outputs you expected?

After renaming the module, I ran WaveDiff. If everything is working correctly, the code should run without errors, and the metrics should have similar logical values as before renaming. The values in the log seem reasonable, but for full assurance, we should compare them to the log from before the renaming.

jeipollack commented 8 months ago

Thanks for this answer. Just a clarification, if you are using the same random seed and configuration for before-the-changes and after-the-changes run the results should be exact, not similar. If they are not exact, it is important to investigate why they are not the same.

nadamoukaddem commented 8 months ago

Yes, by 'similar' I meant the same. As you mentioned, if we are using the same random seed. Thanks for the specifics.

jeipollack commented 8 months ago

Similar /= Same.

Are you going to correct your report, so it's clear which run is the before-changes and after-changes runs? If you don't or can't run the validation tests, then you need to do the two runs and provide the corresponding logs.

nadamoukaddem commented 8 months ago

the log I provided is after-changes run

jeipollack commented 8 months ago

It is difficult as a reviewer to read bits of information across multiple comments. According to the Pull Request template, assignees should provide a summary of their checks in the original post. Thus, it's possible for you to edit it. Again provide a complete report of the validation tests.

Send me a notification when you're done and ready for me to review the Pull Request.

nadamoukaddem commented 8 months ago

metrics_after_changes.log metrics_before_changes.log Done.