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 met.process and dbfiles #3319

Merged
merged 33 commits into from
Sep 21, 2024

Conversation

Sweetdevil144
Copy link
Contributor

@Sweetdevil144 Sweetdevil144 commented Jun 27, 2024

Description

  • Optimized the standardization process by combining the two loops and improving efficiency.
  • Added return statement where we intended to return but didn't implement

Motivation and Context

Removed the redundant loop that iterated over standardize_result to update ready.id. Instead, ready.id is now directly assigned the values from the current iteration's standardize_result if is_standardized is TRUE every time we perform a standardization.

For now, I've commented out the previous changes and will remove them once the current changes are approved.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@github-actions github-actions bot added the Base label Jun 28, 2024
@Sweetdevil144 Sweetdevil144 changed the title Refactor met.process.R to optimize standerdized_result handling Refactor met.process and dbfiles Jun 28, 2024
base/db/R/dbfiles.R Outdated Show resolved Hide resolved
base/db/R/dbfiles.R Outdated Show resolved Hide resolved
is_standardized <- FALSE
}

if(is_standardized) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this changes behavior: previously ready.id would have an entry for each id whether it was standardized or not; this only adds the ids that were standardized. Is that intentional?

(As currently implemented I think ids not standardized would probably have errored out earlier, but this step should not rely on that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented a NA appending too to make sure to append even when we have face a !is_standardized condition. The initial though process I had was whether it would be wise pass the input.id's to .met2model.module and later on to convert_input if we have not standardised our results. Although it seems I failed and ignored the order in array at check which may change the results.

Copy link
Contributor Author

@Sweetdevil144 Sweetdevil144 Jul 31, 2024

Choose a reason for hiding this comment

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

@infotroph Can you take a look into this?

Copy link
Member

Choose a reason for hiding this comment

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

@Sweetdevil144 I was wrong above about the existing behavior -- In the current code standardize_result has an entry for each id, but it is NULL for ids not standardized and therefore gets silently dropped when appending to ready.id. I don't know how advisable that is, but I bet downstream code relies on it.

I'll suggest changes here that eliminate the second for-loop but keep the original behavior -- please edit further if you see problems I missed.

base/db/R/dbfiles.R Outdated Show resolved Hide resolved
base/db/R/dbfiles.R Outdated Show resolved Hide resolved
base/db/R/dbfiles.R Outdated Show resolved Hide resolved
is_standardized <- FALSE
}

if(is_standardized) {
Copy link
Member

Choose a reason for hiding this comment

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

@Sweetdevil144 I was wrong above about the existing behavior -- In the current code standardize_result has an entry for each id, but it is NULL for ids not standardized and therefore gets silently dropped when appending to ready.id. I don't know how advisable that is, but I bet downstream code relies on it.

I'll suggest changes here that eliminate the second for-loop but keep the original behavior -- please edit further if you see problems I missed.

modules/data.atmosphere/R/met.process.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/met.process.R Show resolved Hide resolved
modules/data.atmosphere/R/met.process.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/met.process.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/met.process.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/met.process.R Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Sweetdevil144 Sweetdevil144 left a comment

Choose a reason for hiding this comment

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

I think this PR is ready for review. Mentioning @infotroph and @moki1202 for approval

@infotroph infotroph merged commit 09a58d2 into PecanProject:develop Sep 21, 2024
16 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants