Rename SimPSFToolkit.py module

[nadamoukaddem]

solves #112
Changes Made:

• Renamed SimPSFToolkit.py topsf_simulator.py

• Renamed the class to PSFSimulator

• Updated all package and class imports throughout the codebase

[jeipollack]

I am checking the first few changes and noticed that you didn't change references and imports of the new module name: psf_simulator instead calling it PSFSimulator. PSFSimulator is the name of the class not the module.

Can you please go through and make sure module imports, class imports and doc strings are correct?

Make sure you tested that everything works as expected.

[jeipollack]

There are a few more changes to implement.

[nadamoukaddem]

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]

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]

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]

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]

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]

There are a few more changes to implement.

Changes have been implemented.

[jeipollack]

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

[nadamoukaddem]

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]

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

[nadamoukaddem]

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]

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]

Can you share your validation reports?

[nadamoukaddem]

wf-psf_renameModule.log

[jeipollack]

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]

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]

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]

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

[jeipollack]

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]

the log I provided is after-changes run

[jeipollack]

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]

metrics_after_changes.log
metrics_before_changes.log
Done.

[jeipollack]

Thanks for working on this.