RGDR class implementation

[BSchilperoort]

This PR implements RGDR as a class, with the previously implemented DBSCAN functionality integrated into the class, and with plotting methods available for users to play with the RGDR configuration.

Note that this is a standalone implementation of RGDR, without any knowledge about splits/folds implemented explicitly.

---

Example:

from s2spy import RGDR
rgdr = RGDR(target_timeseries) # instantiate the class

# Fit & apply the RGDR clustering to the training data
clustered_train_data = rgdr.fit(precursor_field_train_data) 

# Apply the clustering to test data
clustered_test_data = rgdr.transform(precursor_field_test_data)

Closes #38

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[geek-yang]

Very nice work! I played with it and checked the code. All look pretty good to me.

But still I have some concerns about the structure of the code. To me it seems too crowded to put RGDR class, all functions doing actual calculations and visualization functions all in one place.

I understand that this fits well with the design if we have a dimensionality folder and put all methods (e.g. rgdr, pca, mca, t-sne...) there, each in a separate file. But if we only want to have RGDR as an independent service and only wrappers for other methods. Then it is better to restructure RGDR module into a few separate files, for readability and maintenance.

We can think about it later when this module grows bigger. For now, just go ahead merge it!

Again, thanks a lot for the endeavor 😄!

[geek-yang]

To me, it makes more sense to place these "plot" codes in a visual.py module and create a method in the RGDR class to call these "plot" codes. This will reduces the size of rgdr.py file and makes it more readable. It is fine for now. But when the code grows again due to the implementation of new methods (e.g. regression map?). We need to restructure the code a bit.

[BSchilperoort]

OK! Yeah we can do this when it's required. Although moving the plotting methods would only reduce the number of lines by ~25. Most of the lines are the docstrings.

[geek-yang]

Nice finding, haha. Maybe we should leave a comment there to push them a bit.

[geek-yang]

Very nice implementation. We can add the center in the visualization in next iteration, together the restructuring of all plot codes if necessary.

[BSchilperoort]

Hi Yang, thanks for the review. RGDR does feel cluttered, however it currently only contains the following;

• spherical_area
• cluster_area
• remove_small_area_clusters
• masked_spherical_dbscan
• _pearsonr_nan
• correlation
• RGDR
  • __init__
  • plot_correlation
  • plot_clusters
  • fit
  • transform

I think the size and structure of the file is fine at the moment. We could move out the correlation method along with the pearsonr_nan function, but that would indeed be appropriate to do when implementing different correlation/regression methods.

We could refactor it to something like;

• spherical_area → utils.py
• cluster_area → utils.py
• remove_small_area_clusters → utils.py
• masked_spherical_dbscan
• _pearsonr_nan → correlation.py
• correlation → correlation.py
• RGDR
  • __init__
  • plot_correlation
  • plot_clusters
  • fit
  • transform

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.9% Coverage
0.0% Duplication