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

spatial sampling: option to limit sampling along X-direction to a given range #1316

Merged
merged 8 commits into from
May 29, 2024

Conversation

jtbuch
Copy link
Collaborator

@jtbuch jtbuch commented Apr 19, 2024

Generalized spatial_sampling.py to also include sampling along X-direction. Possible options include sampling for Z-direction individually or both directions simultaneously.

@slayoo
Copy link
Member

slayoo commented Apr 19, 2024

Thank you @jtbuch!
Let's aim for covering the new logic with unit tests as a part this PR

@jtbuch
Copy link
Collaborator Author

jtbuch commented Apr 19, 2024

Hi @slayoo sure thing! Let me know what I can do to run these tests.

Edit: I figured out the pre-commit test from the FAQ here but still slightly lost about the pylint fail.

@jtbuch
Copy link
Collaborator Author

jtbuch commented Apr 20, 2024

Ah I think I understand what's going on. Will post another commit on Monday after cleaning up the code a little.

@slayoo
Copy link
Member

slayoo commented Apr 25, 2024

@jtbuch
We use pre-commit to ensure consistent formatting of source files (whitespaces, etc).
To make the pre-commit hooks update code automatically before each commit (ensuring pre-commit will pass on CI), you can do:

pip install pre-commit
cd PySDM
pre-commit install

For the test coverage, I suggest to add a new test routine in tests/unit_tests/initialisation/test_spatial_discretisation.py which can be analogous to:
https://github.com/open-atmos/PySDM/blob/main/tests/unit_tests/initialisation/test_spatial_discretisation.py#L42-L57
but testing the newly introduced logic

@jtbuch
Copy link
Collaborator Author

jtbuch commented May 3, 2024

Sounds good, @slayoo! I will update this within the next week.

@slayoo
Copy link
Member

slayoo commented May 3, 2024

Thanks!

@slayoo
Copy link
Member

slayoo commented May 13, 2024

@jtbuch rebasing onto (or merging in) the current main branch should help with the examples-setup failure

@jtbuch
Copy link
Collaborator Author

jtbuch commented May 13, 2024

@slayoo Thank you for the suggestion! I am trying to set up multiple branches for different cases (keep an eye out for more issues 👀) and I will rebase before the next commit.

@jtbuch
Copy link
Collaborator Author

jtbuch commented May 20, 2024

Hi @slayoo I've added a new function locally that tests for both x- and z-directions based on the existing one in https://github.com/open-atmos/PySDM/blob/main/tests/unit_tests/initialisation/test_spatial_discretisation.py#L42-L57

Is there a file that calls test_spatial_discretisation.py where I need to include the function declaration? Or just pushing a commit with the new unit test should suffice?

@slayoo
Copy link
Member

slayoo commented May 20, 2024

Thanks @jtbuch !

Is there a file that calls test_spatial_discretisation.py where I need to include the function declaration? Or just pushing a commit with the new unit test should suffice?

It is enough to add the file to the repo, see https://docs.pytest.org/en/latest/explanation/goodpractices.html#conventions-for-python-test-discovery

@jtbuch
Copy link
Collaborator Author

jtbuch commented May 20, 2024

Ah that link was quite helpful, thanks! I have added the unit test to the repo now after verifying that it works locally.

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.49%. Comparing base (c2b4850) to head (7e58c3d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
+ Coverage   83.47%   83.49%   +0.01%     
==========================================
  Files         362      362              
  Lines        8868     8878      +10     
==========================================
+ Hits         7403     7413      +10     
  Misses       1465     1465              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@slayoo slayoo changed the title Including sampling along X-direction spatial sampling: option to limit sampling along X-direction to a given range May 29, 2024
@slayoo slayoo added this pull request to the merge queue May 29, 2024
@slayoo
Copy link
Member

slayoo commented May 29, 2024

Thanks @jtbuch !

Merged via the queue into open-atmos:main with commit bac3407 May 29, 2024
143 checks 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.

2 participants