-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adding cloud platform computation and PUE #369
base: main
Are you sure you want to change the base?
Conversation
ping @PierreRust @samuelrince if you can kindly review what I proposed :) |
pff, c'est bien gros comme PR , je vais essayer de jeter un oeil 😅 |
Will check ! Thank you :) |
This is a really big PR that's quite hard to read. It looks like it changes all the estimates for all the providers, but I can't tell from the code whether those changes are reasonable. It also introduces a breaking change to the API. It would be better to add new endpoints and deprecate the old one(s), rather than just hard-delete (people need time to update tools, understand the change etc.). Obviously just my 2 cents as I'm not a reviewer, but I would recommend 3 PRs:
Another general tip is to avoid all "unnecessary" renaming. Renaming a class or variable adds a lot of noise to the diff, making it more complex to review. Definitely go ahead and rename things if you think they are wrong (e.g. The same thing goes for refactorings, like the |
Hello,
I’ve finally implemented the methodology we proposed in BoaviztAPI: a bottom-up model to assess the environmental impacts of cloud services.
Key Updates
Breaking Changes
Since these updates affect the computational model, I’ve implemented them gradually to ensure result consistency and verify tests coherency
Next Steps
Let me know your thoughts!