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

Add check for power, frequency and pulse_width dimension #5

Merged
merged 56 commits into from
Feb 2, 2024

Conversation

alessandratrapani
Copy link
Collaborator

@alessandratrapani alessandratrapani commented Jan 18, 2024

  • separate columns for defining the power (frequency/pulse_width) as scalar and as an array.
  • add check for power_per_rois, frequency_per_rois and pulse_width_per_rois dimension to match the number of targeted ROIs if they are defined as lists.
  • add check on power and power_per_roi. If one is defined the other should not, but at least one of them should
  • implement all checks also in the init of the class (I am going to do this in a follow up PR)

alessandratrapani and others added 30 commits January 11, 2024 18:21
@alessandratrapani
Copy link
Collaborator Author

@CodyCBakerPhD Ready to be reviewed

@@ -524,3 +614,54 @@ def add_interval(self, **kwargs):
Add a stimulation parameters for a specific stimulus onset.
"""
super(PatternedOptogeneticStimulusTable, self).add_interval(**kwargs)

if isinstance(kwargs["power"], (list, np.ndarray, tuple)):
Copy link
Member

Choose a reason for hiding this comment

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

Iterable seems to be an accepted type of this argument on the docval side: https://github.com/catalystneuro/ndx-patterned-ogen/pull/5/files#diff-7ca4b1263519dc6a6bcb7281102aa380540eb12e67aeb36d4a46a2ff996bf854R554

As such, what type of iterable is allowed for this field that is not a list, array, or tuple?

Copy link
Member

Choose a reason for hiding this comment

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

Or I'm guessing this is towards the goal of having a more informative error message...

If you forbid iterable types in the docval, what is the error message when you try to call this method using power as if you meant power_per_rois?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TypeError: PatternedOptogeneticStimulusTable.add_interval: incorrect type for 'power' (got 'list', expected 'int or float')
It's informative, but I wanted to suggest to use power_per_roi instead. Do you think it would be better to forbid the type iterable for power, frequency, and pulse_width and just specify to use .._per_roi in the documentation?

@CodyCBakerPhD
Copy link
Member

After meeting with Ryan, he suggested to have 2 different columns for defining the power (frequency/pulse_width) as scalar and as an array.

I agree, this makes it a lot easier to do these checks with, and also easier to read the schema / use the API

@CodyCBakerPhD
Copy link
Member

@alessandratrapani Some various comments ranging from minor to possibly missing checks, otherwise looking close to done

I notice the README is still empty, was that going to be expanded on in a later PR?

@alessandratrapani
Copy link
Collaborator Author

@alessandratrapani Some various comments ranging from minor to possibly missing checks, otherwise looking close to done

I notice the README is still empty, was that going to be expanded on in a later PR?

Yes, I already open a draft PR (#8 ), I wanted to close this PR and #6 before completing the documentation.

@CodyCBakerPhD CodyCBakerPhD merged commit 042cc16 into main Feb 2, 2024
34 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the validate_power_freq_pw_dimension branch February 2, 2024 15:09
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