Optimize output from RGDR

geek-yang commented 2 years ago

Tiny updates to RGDR module:

Return dataarray with the shape ["anchor_year", "cluster_labels"] to be compatible with sklearn models (require inputs in the format of [samples, features])
Add attributes to the returned values (e.g. latitude/longitude refer to the geographical center of each cluster)
Replace errors with warnings when there is only one cluster found

This PR closes #92 and closes #86

review-notebook-app[bot] commented 2 years ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

semvijverberg commented 2 years ago

Dear @geek-yang,

Small question. If you make a pull request, it means you are asking for a review by a team member right?

I noticed the first two points have been addressed nicely.

Return dataarray with the shape ["anchor_year", "cluster_labels"] to be compatible with sklearn models (require inputs in the format of [samples, features])
Add attributes to the returned values (e.g. latitude/longitude refer to the geographical center of each cluster)

But I did not see any changes in 'files changed', that tackle the last point:

Replace errors with warnings when there is only one cluster found.

Cheers, Sem

-- Ah I know see this PR is 'still a work in progress' and it has not been set to 'Ready for review'!

Learned something again today! :)

geek-yang commented 2 years ago

Dear @geek-yang,

Small question. If you make a pull request, it means you are asking for a review by a team member right?

I noticed the first two points have been addressed nicely.

Return dataarray with the shape ["anchor_year", "cluster_labels"] to be compatible with sklearn models (require inputs in the format of [samples, features])

Add attributes to the returned values (e.g. latitude/longitude refer to the geographical center of each cluster)

But I did not see any changes in 'files changed', that tackle the last point:

Replace errors with warnings when there is only one cluster found.

Cheers, Sem

-- Ah I know see this PR is 'still a work in progress' and it has not been set to 'Ready for review'!

Learned something again today! :)

Haha, thanks for your early review @semvijverberg . Indeed it is still a working progress. When it is ready for review, I will click the button ready for review and add you as the reviewer (see the right corner). Then you can check the code, leave your comment and let me know if you give me a pass or not. It will be done soon. But thanks for the early notice 😆 .

semvijverberg commented 2 years ago

Hey @geek-yang,

Small remark. The suggestion to remove the intervals is not really needed. In a larger pipeline there is nothing wrong with not finding cluster at a specific lag.

Hence, my suggestion would be to remove the sentence "Please remove these intervals from the model before continuing." at line 118.

It is my opinion that after this minor change the PR can be merged.

Thanks for the work:)