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

Update installation and general code clean-up #13

Merged
merged 24 commits into from
Jul 15, 2024
Merged

Update installation and general code clean-up #13

merged 24 commits into from
Jul 15, 2024

Conversation

CharlesPageot
Copy link
Collaborator

@CharlesPageot CharlesPageot commented Jul 10, 2024

Summary

@CharlesPageot CharlesPageot requested a review from jcohenadad July 10, 2024 17:30
@CharlesPageot CharlesPageot requested a review from po09i July 11, 2024 20:25
README.md Outdated Show resolved Hide resolved

The _analytical_case.py_ script allows for comparaison between simulated and analytical results for a spherical and cylindrical phantom.
The _analytical_cases_ command allows for comparaison between simulated and analytical results for a spherical and cylindrical phantom.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The _analytical_cases_ command allows for comparaison between simulated and analytical results for a spherical and cylindrical phantom.
The `analytical_cases` command allows for comparaison between simulated and analytical results for a spherical and cylindrical phantom.

Copy link
Member

Choose a reason for hiding this comment

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

it's not very clear to me what the function actually does. Can you give an example of figure output? Also, what should be my input here? it seems that -t and -b are not image inputs, so I'm puzzled as to what I'm supposed to input...?

Copy link
Collaborator Author

@CharlesPageot CharlesPageot Jul 12, 2024

Choose a reason for hiding this comment

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

right, i'll add an example of figures output. the inputs -t and -b are defined in the inputs as the geometry type and the buffer value respectively.

README.md Outdated Show resolved Hide resolved
README.md Outdated
```


### fft_simulation
### compute_fieldmap
Copy link
Member

Choose a reason for hiding this comment

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

I would definitely place this one before. The logical order is to first compute the fieldmap, and then run analytical_cases, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed. will switch the two

@jcohenadad
Copy link
Member

@CharlesPageot in the future I suggest you pick a branch name that is less generic, see our conventions

README.md Outdated Show resolved Hide resolved

Bz_in = np.where(mask, Bz_in, 0)
Bz_out = np.where(~mask, Bz_out, 0)
Bz_in = np.zeros(self.matrix)

Bz_analytical = Bz_out + Bz_in
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of Bz_in? It's only a matrix of zeros.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will remove

@@ -114,17 +209,21 @@ def main(geometry_type, buffer):

# plot sections of the chi distribution
fig, axes = plt.subplots(2, 3, figsize=(10, 5), dpi=120)
Copy link
Member

Choose a reason for hiding this comment

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

A plot () function should be integrated for each class instead of having an if statement with two very similar functions. You could even create a PArent class to both where you define the plot function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created a parent class for plotting

CharlesPageot and others added 3 commits July 12, 2024 11:57
Co-authored-by: Julien Cohen-Adad <[email protected]>
Co-authored-by: Julien Cohen-Adad <[email protected]>
Co-authored-by: Julien Cohen-Adad <[email protected]>
@CharlesPageot CharlesPageot changed the title Address comments from #12 and update installation Update installation and general code clean-up Jul 15, 2024
@CharlesPageot CharlesPageot merged commit 2f81436 into main Jul 15, 2024
1 check passed
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.

4 participants