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

Allow passing multi-index dimension to spatial_analogs #758

Merged
merged 2 commits into from
Jun 29, 2021
Merged

Conversation

huard
Copy link
Collaborator

@huard huard commented Jun 28, 2021

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • bumpversion (major / minor / patch) has been called on this branch
  • Tags have been pushed (git push --tags)

What kind of change does this PR introduce?

  • Add support for multi-index in spatial_analogs

Does this PR introduce a breaking change?

Yes, dist_dim will not accept lists of dimensions anymore. Users are expected to create multi-indexes themselves.

Other information:

…dist_dim`. `api_break`: `dist_dim` does not support sequence of dimensions. Users are expected to do the stacking themselves
@huard huard requested a review from aulemahal June 28, 2021 20:22
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

Thanks!

We might have had a misunderstanding, the dimension to rename is the dist one. Observations are along this one, while "indices" or "variables" along the other one (which was indices before). It doesn't really make sense to have a different number of variables when comparing, but it's ok to have a different number of observations!

@huard
Copy link
Collaborator Author

huard commented Jun 29, 2021

Wait, are you saying that for all that time, the code imposed target and candidate to have the same number of obs ?

@aulemahal
Copy link
Collaborator

Haha, yes.

@huard
Copy link
Collaborator Author

huard commented Jun 29, 2021

Well, this shows it hasn't been used much so far... I think I've fixed it now.

@huard huard merged commit dee4744 into master Jun 29, 2021
@huard huard deleted the fix-743 branch June 29, 2021 19:45
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.

Problems with MultiIndexes
2 participants