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

Ticket/PSB-130: #2689

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Ticket/PSB-130: #2689

merged 1 commit into from
Jun 26, 2023

Conversation

morriscb
Copy link
Contributor

@morriscb morriscb commented Jun 14, 2023

Add stimulus_name and image_set to task_parameters output.

Move code block to produce stimulus_name to utility function.
Update custom NWB schema.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@morriscb morriscb changed the base branch from master to rc/2.15.2 June 14, 2023 17:34
@morriscb morriscb marked this pull request as ready for review June 15, 2023 17:32
@morriscb morriscb requested a review from aamster June 15, 2023 17:32
"""
Get the image stimulus name by parsing the file path of the image set.

If no image set, check for gratings and return behavior is not found.
Copy link
Contributor

Choose a reason for hiding this comment

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

fix typo

@@ -44,7 +50,8 @@ def __init__(self,
stimulus: str,
stimulus_distribution: StimulusDistribution,
task_type: TaskType,
n_stimulus_frames: int):
n_stimulus_frames: int,
stimulus_name: str = Optional[None]):
Copy link
Contributor

Choose a reason for hiding this comment

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

think you meant Optional[str] = None ??

@@ -104,6 +113,14 @@ def task(self) -> TaskType:
def n_stimulus_frames(self) -> int:
return self._n_stimulus_frames

@property
def stimulus_name(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

you have the type as Optional[str] above ??

return self._stimulus_name

@property
def image_set(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

it is a string ??

@@ -63,3 +64,35 @@ def calculate_monitor_delay(sync_file: SyncFile,
warnings.warn(warning_msg)

return delay


def get_image_stimulus_name(stimulus_file: BehaviorStimulusFile) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

please add this as a method on BehaviorStimulusFile as there similar methods there

@@ -44,7 +50,8 @@ def __init__(self,
stimulus: str,
stimulus_distribution: StimulusDistribution,
task_type: TaskType,
n_stimulus_frames: int):
n_stimulus_frames: int,
stimulus_name: str = Optional[None]):
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this can never be null so why is it optional ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When loading from an already created NWB file, this value will not exist hence the default of None.

@@ -167,7 +185,8 @@ def from_stimulus_file(
stimulus=stimulus,
stimulus_distribution=stimulus_distribution,
task_type=task,
n_stimulus_frames=n_stimulus_frames
n_stimulus_frames=n_stimulus_frames,
stimulus_name=stimulus_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

get rid of trailing comma !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I like it 😅 (it's removed)

@@ -273,6 +273,10 @@ class BehaviorTaskParametersSchema(RaisingSchema):
doc='Total number of stimuli frames',
required=True,
)
stimulus_name = fields.String(
doc="Name of the stimulus file set presented",
required=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing comma

@morriscb morriscb requested a review from aamster June 22, 2023 19:08
@@ -203,6 +203,38 @@ def session_type(self) -> str:
"""
return self._retrieve_from_params("stage")

@property
def image_stimulus_name(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

@morriscb How is this different from https://github.com/AllenInstitute/AllenSDK/blob/rc/2.15.2/allensdk/brain_observatory/behavior/behavior_project_cache/tables/util/metadata_parsers.py#L35 ? It might lead to discrepancies if we're using multiple code paths to grab the same thing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The returns are slightly different here. The code you link returns the specific image set shown (e.g. image_A). The image_stimulus_name return gives the full name of the image file (e.g. Natural_Images_Lum_Matched_set_ophys_6_2017) so they are slightly different in their returns.

@@ -203,6 +203,38 @@ def session_type(self) -> str:
"""
return self._retrieve_from_params("stage")

@property
def image_stimulus_name(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be named just stimulus_name since it might be e.g. gratings

@morriscb morriscb merged commit ebac752 into rc/2.15.2 Jun 26, 2023
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