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

Refactor plot_n_important_coeffs, unexport others #68

Merged
merged 13 commits into from
Jan 17, 2024
Merged

Refactor plot_n_important_coeffs, unexport others #68

merged 13 commits into from
Jan 17, 2024

Conversation

Enchufa2
Copy link
Member

@Enchufa2 Enchufa2 commented Jan 15, 2024

Partial work towards #56.

So far, I've refactored the plot_n_important_coeffs just as a nn2poly plot method, and put that together with predict. We still need to

  • Add an example.
  • I'd like to get rid of tidytext. It's weird to have that package in suggests just for those two small things.

Please, address this in this branch while I think a little bit more about the other plot functions.

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (45ca38d) 79.93% compared to head (b6eb121) 79.90%.

❗ Current head b6eb121 differs from pull request most recent head 2fcb13e. Consider uploading reports for the commit 2fcb13e to get more accurate results

Files Patch % Lines
R/nn2poly_methods.R 92.94% 6 Missing ⚠️
R/plot_taylor_and_activation_potentials.R 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
- Coverage   79.93%   79.90%   -0.04%     
==========================================
  Files          16       16              
  Lines         648      652       +4     
==========================================
+ Hits          518      521       +3     
- Misses        130      131       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Enchufa2
Copy link
Member Author

About plot_diagonal: this is just a scatter plot with an abline. It has been a useful convenience wrapper for us so that all these figures in the papers look the same. Two options here:

  1. Simply unexport this function, just like with eval_poly.
  2. If you think this is really important for the user, then we could integrate it into plot.nn2poly: if y is missing, then we plot the coefficients. If y is a NN object (of one of the supported classes), then we plot the diagonal.

@Enchufa2
Copy link
Member Author

For plot_taylor_and_activation_potentials... I have plans. But they are quite involved, and the goal is to hit CRAN ASAP, so I'd say:

  1. let's unexport both plot_diagonal and plot_taylor_and_activation_potentials for now, keeping the manual pages and using them via ::: in the vignettes;
  2. let's make a first release;
  3. we will decide about plot_diagonal and rework plot_taylor_and_activation_potentials in a future release.

@Enchufa2 Enchufa2 changed the title Refactor plot functions Refactor plot_n_important_coeffs, unexport others Jan 17, 2024
@Enchufa2 Enchufa2 added this to the Initial CRAN submission milestone Jan 17, 2024
@moralapablo moralapablo marked this pull request as ready for review January 17, 2024 17:33
@Enchufa2
Copy link
Member Author

There's an issue with latest Posit Package Manager sync with CRAN. Tests are fine. Merging.

@Enchufa2 Enchufa2 merged commit 6a0584a into master Jan 17, 2024
8 of 13 checks passed
@Enchufa2 Enchufa2 deleted the plot branch January 18, 2024 16:14
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