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

Inquiry Range Sampler #374

Open
wants to merge 5 commits into
base: 2.0.0
Choose a base branch
from
Open

Inquiry Range Sampler #374

wants to merge 5 commits into from

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Jan 21, 2025

Overview

Added a Sampler for filtering the data by inquiry range and series range.

Ticket

https://www.pivotaltracker.com/story/show/188673150

Contributions

  • Refactored samplers into their own directory.
  • New sampler for filtering data.
  • Added sampler arguments to the Task Factory so users could configure the parameters for a Sampler.
  • Command line support to the simulator to accept sampler and sampler args parameters.
  • Added a sampler pulldown menu to the simulator GUI.
  • Added a new GUI component to generate input fields for a given object and used it on the simulator GUI to dynamically configure the selected sampler.

Test

  • Created and ran unit tests.
  • Ran the simulator using various configurations of samplers and inputs to confirm that the correct values were being used.

@lawhead lawhead requested a review from tab-cmd January 21, 2025 18:35
Copy link
Contributor

@tab-cmd tab-cmd left a comment

Choose a reason for hiding this comment

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

Codacy made a few minor suggestions (typing, etc.) that I agree with; otherwise, it looks great. I'm excited to try it with the group later today.

sampling_strategy=classify(
sim_args['sampler']),
task=SimulatorCopyPhraseTask,
sampler_args=json.loads(sim_args['sampler_args']))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this a method. We could add some parameter validation or helpful logging to it.

@@ -43,6 +45,8 @@ For example,
- `m`: Path to a pickled (.pkl) signal model. One or more models can be provided.
- `n`: Number of simulation runs
- `o`: Output directory for all simulation artifacts.
- `s`: Sampling strategy to use; by default the TargetNonTargetSampler is used. The value provided should be the class name of a Sampler.
- `sampler_args`: Arguments to pass in to the selected Sampler. Some samplers can be customized with further parameters. These should be structured as a JSON dictionary mapping keys to values. For example: `--sampler_args='{"inquiry_end": 4}'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only parameter that could change in sampling? It may be helpful to have a custom_args= that could be used to modify other parameters in the future, too.

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.

2 participants