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

#746 - Reorganise files #940

Merged
merged 11 commits into from
Oct 7, 2024
Merged

#746 - Reorganise files #940

merged 11 commits into from
Oct 7, 2024

Conversation

nikosbosse
Copy link
Contributor

Description

This PR closes #746.

This PR moves everything around and reshuffles the deck. In particular, it moves everything related to a forecast class to its own class-forecast-<type>.R file and everything related to its metrics to a metrics-type.R file.

While I was there I took the chance to make some changes to various functions.
(I am indeed kidding, I did not do that).
I didn't make any changes to actual functions, so this is really just moving around code.

So far I didn't move most of the tests yet - I first wanted to have a pair of eyes on the file structure before I start moving tests back and forth.

Additional thoguhts:

  • We could move the plot functions to the corresponding functions instead of collecting all of them in its own file. No strong opinions here.
  • Some areas for future refactoring
  • Discussion for another issue/PR, but I did realise again how many internal functions are used by various forecast classes. Exporting all of these functions seems like a real cost.
    • Would it be so bad if others had to call scoringutils::: in their code (once anyone actually wants to implement something that we think can't sit in scoringutils and has to go to CRAN we can still rethink that)
    • If we're exporting all these functions, maybe we could at least prefix them with something?

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@nikosbosse nikosbosse marked this pull request as draft October 3, 2024 21:32
@nikosbosse nikosbosse requested a review from seabbs October 3, 2024 21:32
@seabbs
Copy link
Contributor

seabbs commented Oct 4, 2024

We could move the plot functions to the corresponding functions instead of collecting all of them in its own file. No strong opinions here.

I like this idea.

If we're exporting all these functions, maybe we could at least prefix them with something?

Agree. Needing to export them is maybe good motivation for reducing the number of them that are needed.

R/get-pit.R Show resolved Hide resolved
R/score.R Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looks really great. Much nicer. A few minor points/questions

@nikosbosse
Copy link
Contributor Author

There is currently nothing in the file naming to indicate when somerthing is a normal function vs a s3 default method. We might want to do this to make it ewasieer for people to find things which they needed to write new methods for.

I see the point, but there isn't quite a clear-cut distinction. We could add something like "-generic" to some of the files, but most files don't only contain an S3 generic. Unsure. Inclined to merge this now and revisit later?

I updated the test files and am sure I missed something here in there. There are a lot of tests and some of them are probably duplicated and should be cleaned up. But maybe that's a project for another day.

@nikosbosse nikosbosse marked this pull request as ready for review October 6, 2024 20:53
@nikosbosse nikosbosse requested a review from seabbs October 6, 2024 20:53
@nikosbosse
Copy link
Contributor Author

If we're exporting all these functions, maybe we could at least prefix them with something?

I haven't done this yet and I'm leaning towards not doing that until after the next CRAN release. We have a hard deadline for the CRAN release on October 31. And my impression is that there is some internal refactoring to do that might make some of those functions obsolete after all so my preference would be to avoid an export --> deprecate cycle here if possible.

@seabbs
Copy link
Contributor

seabbs commented Oct 7, 2024

Unsure. Inclined to merge this now and revisit later?

Okay

We have a hard deadline for the CRAN release on October 31

Ah do we. News to me!

@seabbs seabbs merged commit 4371a23 into main Oct 7, 2024
9 checks passed
@seabbs seabbs deleted the reorganise-files branch October 7, 2024 09:23
@nikosbosse
Copy link
Contributor Author

Ah do we. News to me!

ooh, sorry, I thought I had mentioned that more explicitly at some point. This is what #920 was about - they now extended the deadline to October 31.

But then again we might already be slightly behind our own Q1/2024 deadline :D

@nikosbosse
Copy link
Contributor Author

So basically the deadline is we need to fix the author names - but I'd really like to get all the changes up by the end of October as well.

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.

Rename and reorganise files
2 participants