Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track generation for arc_02 and arc_03 #241

Merged
merged 10 commits into from
Apr 27, 2023
Merged

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Feb 28, 2023

Continues #240 |

Adds arc_02 and arc_03 to the list of tests which are now covered by the input data regeneration scripts.

Modifications on top of #236:

  • run_bscan now handles different spatial objects, obstacle radii, and when illsetup is required on top of the usual filesetup.
  • config_XX.yaml and input_file_XX.m are added to the data/input_generation folder appropriately
  • generate_input_data.py has been split into bscan_arguments.py and generation_data.py containing classes of the same name, to separate functionality.
  • Reduces the number of variables and input parameters that were stored more than once across the test classes (this is redundant because the classes now no longer exist in the base branch)
  • test_regen.py will run tests in numerical order when called locally, which is nice.

Force-push changes here 👉 #241 (comment)

Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

Messy review that I had started already and then just ... didn't click "Submit"?

Let me send the comments now, even though they may become obsolete after #236 is tweaked.

.isort.cfg Outdated Show resolved Hide resolved
tdms/tests/system/README.md Outdated Show resolved Hide resolved
tdms/tests/system/data/input_generation/bscan_arguments.py Outdated Show resolved Hide resolved
@@ -17,4 +17,5 @@ tests:
input_generation:
input_file: input_file_01.m
spatial_obstacles: ["fs", "cyl"]
illumination_input_file:
illsetup:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the pedestrian question: is illsetup used in TDMS proper?

... Because I preferred illumination_input_file for its explicitness. Perhaps illumination_setup (whole words).

tdms/tests/system/data/input_generation/bscan_arguments.py Outdated Show resolved Hide resolved
tdms/tests/system/README.md Outdated Show resolved Hide resolved
tdms/tests/system/data/input_generation/bscan_arguments.py Outdated Show resolved Hide resolved
Base automatically changed from wgraham-input_data_generation to main April 25, 2023 08:08
- Updates run_bscan.m to handle illsetup and non-cyl obstacles
- Updates config files to use illsetup rather than illumination_file key for brevity
- illsetup is a bool flag, not a string now
- scattering_matrix handles the setup of spatial objects via a string option
- generate_test_input can handle regenerating data according to illsetup or no illsetup, and different spatial objects
- doc/developers directs readers to config_01.yaml for explanation of the config files
- Docstrings are added to run_bscan
- Default values are placed into a dictionary in generate_test_input.py to avoid duplication and account for changes later.
Rename illfile_required to illsetup in run_bscan wrapper.
- There are distinct usage cases (sometimes an illfile has to be created, other times it is provided as a .mat input.)
- As such, the name must be explicit in this case.
@willGraham01
Copy link
Collaborator Author

willGraham01 commented Apr 25, 2023

Rebased this guy now too. In response to the previous comments:

The illsetup matter is an awkward inconvenience however: illsetup is one of the modes iteratefdtd_matrix can be run in. In this mode, you provide an input with particular source fields and MATLAB goes away and computes it. However, you then need to pass in the filename that contains this source data back into iteratefdtd_matrix when setting up the reference inputs.

However, you can also just provide iteratefdtd_matrix with the source data straight away, in which case you don't need to run in illsetup but do need to provide an illumination file. So "illfile_required" doesn't actually distinguish between when we do/do not need illsetup and vice-versa.

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@669121b). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #241   +/-   ##
=======================================
  Coverage        ?   47.56%           
=======================================
  Files           ?       63           
  Lines           ?     7804           
  Branches        ?        0           
=======================================
  Hits            ?     3712           
  Misses          ?     4092           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@samcunliffe
Copy link
Member

@codecov probably did the comparison with main before a coverage report had been uploaded. I expect it will make sense when/if you push anything else here and trigger a rebuild.

Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

👍

willGraham01 and others added 2 commits April 27, 2023 09:38
Co-authored-by: Sam Cunliffe <samcunliffe@users.noreply.github.com>
@samcunliffe samcunliffe merged commit 6d9a89a into main Apr 27, 2023
13 checks passed
@samcunliffe samcunliffe deleted the wgraham-input_data_arc0203 branch April 27, 2023 12:41
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.

None yet

2 participants