Test aggregated cellbox #70
Conversation
…egated_cellbox Signed-off-by: hjabbot <42661345+hjabbot@users.noreply.github.com>
…un by TestAutomator
| self.assertTrue(self.arbitrary_agg_cb.contains_point(50, 50)) | ||
| self.assertTrue(self.equatorial_agg_cb.contains_point(0, 50)) | ||
| self.assertTrue(self.meridian_agg_cb.contains_point(50, 0)) | ||
| # self.assertTrue(self.antimeridian_agg_cb.contains_point(50, 180)) |
There was a problem hiding this comment.
Any reason this ones commented out?
There was a problem hiding this comment.
There's a bug in aggregated_cellbox.py, basically the contains_point() method doesn't account for the antimeridian. It seems wrong to fix that and also be writing the test suite in the same PR, so I commented that one out, so that when we fix the bug, we can just uncomment it to test it works.
Happy to fix the bug in this PR if you prefer though
There was a problem hiding this comment.
I'd say uncomment the test out in this PR. If it's failing on the dev branch, we should reflect that in our tests. That being said, I don't really have a problem with the fix going into the same PR if it's fairly simple to do.
There was a problem hiding this comment.
Ok, just fixed this up and am running tests now. How about the other bugs noted in the PR?
There was a problem hiding this comment.
There was a problem hiding this comment.
Unsure about the deallocate_cellbox one - Think it's best to ask @ayat-khairy to see if this is functioning as she expected.
I don't think the 'id' should be part of the agg_data. I'm a little cautious that removing this here might break something though - but I suppose we'll have the tests now (or soon) to catch it.
Happy to change some function names in aggregrated_cellbox.py if it would increase clarity
Last one's just been fixed :)
There was a problem hiding this comment.
The regression tests still pass after removing 'id' from the agg_data. I think it's not an issue because the way it currently works is to write out the agg_cellbox in json, and the 'id' key/val pair gets overwritten by the line cell_json['id'] = self.get_id(), which outputs the same key/val pair anyway
There was a problem hiding this comment.
Just renamed set_boundary to set_bounds to keep consistency within meshiphi. Luckily, it's not called anywhere in the code with the exception of the test suite that this PR addresses
MeshiPhi Pull Request Template
Date: 2024-05-23
Version Number: 2.1.7
Description of change
Added test suite for aggregated_cellbox.py.
The following bugs were found that should be addressed in future PR's once the complete test suite is done.- aggregated_cellbox.py: load_from_json() includes 'id' as part of agg_data- aggregated_cellbox.py: inconsistent naming between set_boundary and get_bounds methods- aggregated_cellbox.py: contains_point() does not account for antimeridianEDIT: Addressed these bugs in this PR
Fixes #48
Testing
To ensure that the functionality of the MeshiPhi codebase remains consistent throughout the development cycle a testing strategy has been developed, which can be viewed in the document
test/testing_strategy.md.This includes a collection of test files which should be run according to which part of the codebase has been altered in a pull request. Please consult the testing strategy to determine which tests need to be run.
My changes have not altered any of the files listed in the testing strategy
My changes result in all required regression tests passing without the need to update test files.
passing_tests
My changes require one or more test files to be updated for all regression tests to pass.
Checklist
2.1.xbranch (DO NOT SUBMIT A PR TO MAIN)