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

Gb/rpm h5 excl #19

Merged
merged 5 commits into from
Dec 4, 2019
Merged

Gb/rpm h5 excl #19

merged 5 commits into from
Dec 4, 2019

Conversation

grantbuster
Copy link
Member

Refactor of RPM outputs handler with h5 exclusions. Added basic RPM baseline validation tests.

@@ -349,7 +348,7 @@ def _dist_rank_filter(self, iterate=True, **kwargs):
c_centroids = clusters.cluster_coordinates
dist_i = np.linalg.norm(c_centroids - centroids)
rmse_i = np.mean((c_coeffs - coeffs) ** 2) ** 0.5
if (dist_i < dist and rmse_i < rmse):
if (dist_i <= dist and rmse_i <= rmse):
Copy link
Member Author

Choose a reason for hiding this comment

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

@MRossol, FYI this was giving me a ton of trouble in my test cases. I'm not 100% sure how this works but dist_i and rmse_i were both 0 in the first iteration so the while loop hung indefinitely. Making these statements <= worked. I suspect the issue is something to do with the tiny domain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that is strange but seems like a simple fix.

Copy link
Collaborator

@MRossol MRossol left a comment

Choose a reason for hiding this comment

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

One comment and one request

  • Can we pull in the updated pull_request_test.yml from the reeds branch and also move it to bin, this should prevent a future merge conflict when we rebase reeds

@@ -349,7 +348,7 @@ def _dist_rank_filter(self, iterate=True, **kwargs):
c_centroids = clusters.cluster_coordinates
dist_i = np.linalg.norm(c_centroids - centroids)
rmse_i = np.mean((c_coeffs - coeffs) ** 2) ** 0.5
if (dist_i < dist and rmse_i < rmse):
if (dist_i <= dist and rmse_i <= rmse):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that is strange but seems like a simple fix.

@@ -715,9 +724,11 @@ def _single_excl(cluster_id, clusters, excl_fpath, techmap_fpath,
n_inclusions = np.zeros((len(locs), ), dtype=np.float32)
n_points = np.zeros((len(locs), ), dtype=np.uint16)

techmap = RPMOutput._get_tm_data(techmap_fpath, techmap_dset,
excl = ExclusionMask.from_dict(excl_fpath, excl_dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more pythonic and safer to run this as a with so the context manager will properly close the file in case anything goes wrong

@grantbuster
Copy link
Member Author

@MRossol, two commits to deal with the github actions yml and the exclusions context manager.

MRossol
MRossol previously approved these changes Dec 4, 2019
Copy link
Collaborator

@MRossol MRossol left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@MRossol MRossol left a comment

Choose a reason for hiding this comment

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

f'ing workflow file

@grantbuster grantbuster merged commit 92f928a into master Dec 4, 2019
@grantbuster grantbuster deleted the gb/rpm_h5_excl branch December 4, 2019 16:18
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