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

Refactor read_qdm_harvard function #11

Merged
merged 16 commits into from
Mar 19, 2024
Merged

Refactor read_qdm_harvard function #11

merged 16 commits into from
Mar 19, 2024

Conversation

YagoMCastro
Copy link
Contributor

@YagoMCastro YagoMCastro commented Mar 18, 2024

Created two private functions in order to make the code easier to read

Relevant issues/PRs:

Copy link

welcome bot commented Mar 18, 2024

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

No matter what, we are really grateful that you put in the effort to do this!

@YagoMCastro YagoMCastro marked this pull request as ready for review March 18, 2024 14:04
@YagoMCastro YagoMCastro changed the title Yago read qdm harvard Refactor read_qdm_harvard function Mar 18, 2024
magali/_io.py Outdated
x = np.arange(shape[1]) * spacing
y = np.arange(shape[0]) * spacing
z = np.full(shape, sensor_sample_distance)
data = vd.make_xarray_grid(
(x, y, z),

Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove these blank lines.

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

Well done @YagoMCastro! Made a few suggestions to improve the names and wording in the docstrings. Code itself is good and helps simplify the function. Thanks!

magali/_io.py Outdated Show resolved Hide resolved
magali/_io.py Outdated Show resolved Hide resolved
magali/_io.py Outdated Show resolved Hide resolved
magali/_io.py Outdated Show resolved Hide resolved
magali/_io.py Outdated
return data


def _set_qdm_data_grid(contents):
Copy link
Member

Choose a reason for hiding this comment

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

How about _extract_data_qdm_harvard? It's not really "setting" the data but extracting it from the contents dict.

magali/_io.py Outdated
return (x, y, z), data_names, bz


def _create_grid_qdm_data(coordinates, data_names, bz, path):
Copy link
Member

Choose a reason for hiding this comment

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

How about _create_qdm_harvard_grid?

magali/_io.py Outdated

def _create_grid_qdm_data(coordinates, data_names, bz, path):
"""
Grid QDM microscopy data in the Harvard group format.
Copy link
Member

Choose a reason for hiding this comment

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

Update accordingly.

magali/_io.py Outdated
Comment on lines 89 to 91
This is the file type used by Roger Fu's group to distribute QDM data. It's
a Matlab binary file that has the data and some information about grid
spacing.
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant to this functions.

Copy link
Member

Choose a reason for hiding this comment

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

This functions makes the xarray.Dataset and sets appropriate metadata.

magali/_io.py Outdated Show resolved Hide resolved
magali/_io.py Outdated Show resolved Hide resolved
YagoMCastro and others added 10 commits March 18, 2024 12:51
Co-authored-by: Leonardo Uieda <leo@uieda.com>
Co-authored-by: Leonardo Uieda <leo@uieda.com>
Co-authored-by: Leonardo Uieda <leo@uieda.com>
Co-authored-by: Leonardo Uieda <leo@uieda.com>
Co-authored-by: Leonardo Uieda <leo@uieda.com>
Co-authored-by: Leonardo Uieda <leo@uieda.com>
@leouieda
Copy link
Member

Well done! Merging.

@leouieda leouieda merged commit f7e41b0 into main Mar 19, 2024
15 checks passed
@leouieda leouieda deleted the yago-read_qdm_harvard branch March 19, 2024 11:44
Copy link

welcome bot commented Mar 19, 2024

🎉 Congrats on merging your first pull request and welcome to the team! 🎉

If you would like to be added as a author on the Zenodo archive of the next release, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

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.

None yet

2 participants