Skip to content

Multi-Output helper function #345

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

Merged
merged 18 commits into from
Jul 30, 2021
Merged

Multi-Output helper function #345

merged 18 commits into from
Jul 30, 2021

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Jul 27, 2021

Implements a function to make it possible to work with our input types for multi-output GPs more straightforwardly in the isotopic case.

It's entirely possible that we'll want more helper functions as we move forwards, so this should be seen as a first step in that direction, rather than something that will solve all useability problems.

edit: I've added a note to the docs. Unless I'm missing something obvious, we don't currently have a multi-output example, so I'm not going to create one in this PR to showcase this functionality.

edit2: also note that I'm testing only via doctests. It feels a bit weird, but it's not obviously to me that it's obviously a bad idea, so maybe it's okay?

willtebbutt and others added 2 commits July 27, 2021 22:50
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@theogf
Copy link
Member

theogf commented Jul 27, 2021

Did not go into details but the obvious edge case not considered here is if N == P

Co-authored-by: st-- <st--@users.noreply.github.com>
Copy link
Member

@st-- st-- left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I think it'd be super helpful to include a concrete example with explicit numbers in the docstrings, as even being somewhat familiar with all this it keeps doing my head in. Do doctests count towards code coverage? It might make more sense to move the "consistency checks" to actual tests.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@willtebbutt
Copy link
Member Author

Eurgh -- looks like 1.3 doesn't like the doctests. I feel like we must have encountered this before, any thoughts @theogf @st-- ?

st--
st-- previously approved these changes Jul 29, 2021
Copy link
Member

@st-- st-- 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 to me; no idea why the doctest fails - it should be caught by the filter! @theogf was going to have a look into that

@theogf
Copy link
Member

theogf commented Jul 29, 2021

Just trying something now

Co-authored-by: st-- <st--@users.noreply.github.com>
@theogf
Copy link
Member

theogf commented Jul 29, 2021

It works locally at least!

Co-authored-by: st-- <st--@users.noreply.github.com>
@willtebbutt
Copy link
Member Author

@theogf your regex magic seems to have sorted it. Have bumped patch -- will merge once CI passes.

@willtebbutt willtebbutt merged commit d381a68 into master Jul 30, 2021
@willtebbutt willtebbutt deleted the wct/multi-output-helpers branch July 30, 2021 10:28
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