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

Implemented #144, partial volume via settings #145

Merged
merged 3 commits into from
Jun 23, 2022

Conversation

jnoelke
Copy link
Collaborator

@jnoelke jnoelke commented Jun 1, 2022

I implemented #144. The partial volume effect within the device can now be adapted in the settings. The tag CONSIDER_PARTIAL_VOULME is now always necessary in the volume_creation_settings.

Copy link
Collaborator

@jgroehl jgroehl left a comment

Choose a reason for hiding this comment

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

I agree that this is a good idea.
But I am not sure that implementing it this way is the best idea, as it will lead to a crash if the CONSIDER_PARTIAL_VOLUME tag is not set.

I'd suggest first checking if the field is in the settings dictionary. If it is not, either throw a meaningful error or (preferably?) assume a default value (I think we assume this to be False in the volume creator if not set)?

If I am mistaken: Should we include a default behaviour?

@kdreher
Copy link
Collaborator

kdreher commented Jun 9, 2022

I would suggest the following:

  • our default is that no structures consider the partial volume effect. This can be implemented here where the Tag CONSIDER_PARTIAL_VOLUME in the GeometricalStructure base is queried in the structure settings and it is only set to True if it exists and is set to True by the user.
  • We introduce a new Tag CONSIDER_PARTIAL_VOLUME_IN_DEVICE which is first checked for in the component settings during the update_settings_for_use_of_model_based_volume_creator method. Then, the Tag CONSIDER_PARTIAL_VOLUME of both of the device structures (US_Gel and membrane) is also only set to True if CONSIDER_PARTIAL_VOLUME_IN_DEVICE exists and it is set to True.

Then, the default for everything is no PV, but the user can set it for each structure themselves and for all structures in a device as a whole.

- Implement new default (PV=False)
- Add PV-Tag for device
@jnoelke
Copy link
Collaborator Author

jnoelke commented Jun 13, 2022

I added a new commit that should implement the behaviour suggested by @kdreher .

@jnoelke jnoelke requested a review from jgroehl June 15, 2022 08:41
@jgroehl
Copy link
Collaborator

jgroehl commented Jun 15, 2022

@kdreher the automatic tests do not seem to be triggered when pushing into develop?

self.partial_volume = single_structure_settings[Tags.CONSIDER_PARTIAL_VOLUME]
if Tags.CONSIDER_PARTIAL_VOLUME in single_structure_settings:
self.partial_volume = single_structure_settings[Tags.CONSIDER_PARTIAL_VOLUME]
else: self.partial_volume = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think our coding convention is to have if else block statements on a new line unless it is an inline if-else. Apart from that the changes look reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I changed this.

@jgroehl jgroehl merged commit 17e02d5 into develop Jun 23, 2022
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.

3 participants