Skip to content

Refactor and add validation for carbon flux#210

Draft
manukala6 wants to merge 4 commits intomainfrom
carbon_flux_validation
Draft

Refactor and add validation for carbon flux#210
manukala6 wants to merge 4 commits intomainfrom
carbon_flux_validation

Conversation

@manukala6
Copy link
Member

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

5: 30,
6: 50,
7: 75,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This table is unneeded, already appears and used in stages.py

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in latest commit



def gadm_carbon_flux(tasks, bbox: Optional[Polygon] = None):
return compute_carbon_flux(tasks, bbox)
Copy link
Collaborator

Choose a reason for hiding this comment

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

gadm_carbon_flux() and compute_carbon_flux() are identical, not sure why you need both. Just remove lines 24-27, so you only have gadm_carbon_flux().

Copy link
Member Author

Choose a reason for hiding this comment

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

removed those lines

}
coord_dict["value"] = values

df = pd.DataFrame(coord_dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace lines 154-167 with a call common_stages.py:create_result_dataframe() (as it was before). No need to duplicate code. This will do all these lines, and also the country processing that you had to add at the end of carbon.py:compute_carbon_flux().

Copy link
Member Author

Choose a reason for hiding this comment

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

switched back to common_stages.py:create_result_dataframe()

# Convert tcd thresholds to percentages (already done in stages.py)
# Convert country codes to alpha3
result_df["country"] = result_df["country"].map(numeric_to_alpha3)
result_df.dropna(subset=["country"], inplace=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just remove lines 62-65, assuming you call common_stages.py:create_result_dataframe() in stages.py, as I suggest.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in latest commit

result_uri = "s3://lcl-analytics/zonal-statistics/admin-carbon.parquet"
funcname = "sum"
@flow(name="Carbon Flux")
def gadm_carbon_flux_flow(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you changed the name of this function, you need to update the call at line 64 in run_updates.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just switched it back to the old name and corrected it accordingly

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.

2 participants