Skip to content
This repository has been archived by the owner on Nov 28, 2023. It is now read-only.

Update DataSet.py #237

Closed
wants to merge 1 commit into from
Closed

Update DataSet.py #237

wants to merge 1 commit into from

Conversation

manonreau
Copy link
Contributor

fix bug
compute get_grid_shape only if grid_shape is not provided

fix bug
compute get_grid_shape only if grid_shape is not provided
@coveralls
Copy link

coveralls commented Jun 29, 2021

Pull Request Test Coverage Report for Build 984135998

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 77.12%

Totals Coverage Status
Change from base Build 741819231: 0.0%
Covered Lines: 1628
Relevant Lines: 2111

💛 - Coveralls

# get grid shape
self.get_grid_shape()

if self.grid_shape is None:
Copy link
Member

Choose a reason for hiding this comment

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

The if self.grid_shape is None condition has been considered in the function get_grid_shape, see the line 824 of the same file.

Copy link
Contributor Author

@manonreau manonreau Nov 10, 2021

Choose a reason for hiding this comment

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

Indeed but then the following elif condition is not correctly placed (line 839). It is more logical to remove the if self.grid_shape is None from that function and to enter the get_grid_shape function only if self.grid_shape does not exist.
This same issue is pointed out in #242

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll close this PR then.

Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
Please have a look at my comment. If you agree with that, then please close/delete this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants