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

draft: [IMP|FIX] Improve performance and fix previous problems #228

Draft
wants to merge 5 commits into
base: 17.0
Choose a base branch
from

Conversation

BonnetAdam
Copy link
Member

Description

  • I've set a fallback reference when none are found so that there is always one that can match the search.
  • I've also changed the way how we recompute in order to gain performance with unwanted delete/create.
  • They are also a bug where in some model record we have an in/out manual but no carbon_factor_id set, so I've implemented a migration to fix all previous data

Related Issues

For the bug where we cannot find a fallback reference, here are the issues: #220

Self proofreading checklist

  • Code Review: Have you reviewed your code to ensure it follows best practices
    and meets the project's coding standards?
  • Testing: Have you tested your changes locally to ensure they work as expected?
  • Documentation: Have you updated any relevant documentation or comments to
    reflect your changes?
  • Pre-commit Checks: Have you ensured that pre-commit checks have automatically
    run upon committing to catch any potential issues?

Test Instructions

I haven't tested it out yet, BCH are going to do a deep down search if everything checked out

Additional Notes

Nope all good

@BonnetAdam BonnetAdam self-assigned this Jan 8, 2025
@sustainabilitybot
Copy link
Collaborator

Hi @jacopobacci, @jguenat, @BonnetAdam,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@jguenat jguenat left a comment

Choose a reason for hiding this comment

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

  • Migration script is okay
  • Performance improvements needs more testing (to do again after basics tests are done)
  • fallback to company is not good (see comment)

I suggest doing separate PR for each point.

Comment on lines 413 to +440
def has_valid_carbon_fallback(self, carbon_type: str):
self.ensure_one()
return self[f"carbon_{carbon_type}_fallback_reference"] and self[
f"carbon_{carbon_type}_fallback_reference"
].has_valid_carbon_value(carbon_type)
field_name = f"carbon_{carbon_type}_fallback_reference"
need_re_call = False
fallback_reference = getattr(self, field_name) or False

has_valid_carbon_value = False
if fallback_reference:
has_valid_carbon_value = (
fallback_reference.has_valid_carbon_value(carbon_type) or False
)

# Condition explained: if fallback_reference is not set and the current record is not the last one in the list of CARBON_MODELS
if (
not fallback_reference
and self._name not in CARBON_MODELS
and self._name != CARBON_MODELS[-1]
):
_logger.warning(
f"Fallback reference for '{carbon_type}' is not set on {self}. Setting it to the company."
)
setattr(self, field_name, self.env.company)
need_re_call = True

if need_re_call:
return self.has_valid_carbon_fallback(carbon_type)

return fallback_reference and has_valid_carbon_value
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the correct place to do that. Did you try with _get_carbon_compute_default_record and/or get_carbon(in/out)_fallback_records() ?

@BonnetAdam BonnetAdam marked this pull request as draft January 24, 2025 08:48
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