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

Multiple rms closest #145

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Multiple rms closest #145

wants to merge 12 commits into from

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented Oct 12, 2021

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <https://github.com/GranthamImperial/silicone/pull/XX>`_) Added feature which does something
- (`#XX <https://github.com/GranthamImperial/silicone/pull/XX>`_) Fixed bug identified in (`#YY <https://github.com/GranthamImperial/silicone/issues/YY>`_)

@znicholls
Copy link
Collaborator Author

@Rlamboll still a draft but wanted to get your thoughts at this stage before I write an example notebook

@codecov-commenter
Copy link

Codecov Report

Merging #145 (34adf58) into master (8d6bbbc) will decrease coverage by 0.62%.
The diff coverage is 88.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   96.98%   96.35%   -0.63%     
==========================================
  Files          23       23              
  Lines        1159     1206      +47     
  Branches      252      257       +5     
==========================================
+ Hits         1124     1162      +38     
- Misses         16       21       +5     
- Partials       19       23       +4     
Impacted Files Coverage Δ
src/silicone/database_crunchers/rms_closest.py 89.51% <88.00%> (-5.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d6bbbc...34adf58. Read the comment docs.

@Rlamboll
Copy link
Collaborator

Hi Zeb, I like the idea of the tool and the syntax for infill multiple seems fine. I find the interrupting loops in the code quite hard to follow, so more comments would be useful, as well as more documentation since it's unusual for a cruncher to have this sort of ability. I'd also do more tests, like a test where it's possible to return the wrong answer for two of the variables without inventing new data - currently you are choosing the right pathway from only one option on the N2O two-pathway use case!

@znicholls
Copy link
Collaborator Author

interrupting loops

Is this a synonym for nested loops?

like a test where it's possible to return the wrong answer for two of the variables without inventing new data

Ok cool will have a think. If you have any more you want to suggest please fire away (if you want to make a PR into this one with the use cases of interest that could also work well).

@Rlamboll
Copy link
Collaborator

No, by interrupting I mean loops featuring continues and breaks. I don't belong to the hard school of "never use these, put it all in an if statement or control your loops" but a few comments would go a long way in increasing readibility.

I'm quite busy now and writing tests for this would be low on my to-do list but I can do it in the distant future.

@znicholls
Copy link
Collaborator Author

No, by interrupting I mean loops featuring continues and breaks. I don't belong to the hard school of "never use these, put it all in an if statement or control your loops" but a few comments would go a long way in increasing readibility.

Ye cool got it

I'm quite busy now and writing tests for this would be low on my to-do list but I can do it in the distant future.

Yep let's leave this sitting as a proof of concept until we find time again

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.

3 participants