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

fix(xpansion): update for improved parameter handling and code refactoring #1865

Merged
merged 9 commits into from
Jan 4, 2024

Conversation

MartinBelthle
Copy link
Contributor

@MartinBelthle MartinBelthle commented Dec 13, 2023

Changes Proposed:

  1. Change "-" to "_" in relative_optimality_gap parameter:

    • Replace hyphen "-" with underscore "_" in the relative_optimality_gap parameter for consistency:
      drop the relative-optimality-gap parameter in the backend and the front end)
  2. Removal of obsolete fields related to r version of Xpansion:

    • Remove obsolete fields associated with the R version of Xpansion to clean up the codebase.
  3. Improvement of field labels for better user comprehension:

    • Enhance field labels according to Thomas B. specifications, to make them more understandable.
  4. Add constraints for numeric fields:

    • Implement constraints on numeric fields to minimize input errors (min values, ranges, steps).
  5. Backend enhancement for empty strings in "additional-constraints" and "yearly-weights" parameters:

    • Modify the backend logic to use an empty string when "additional-constraints" and "yearly-weights" parameters are
      not included in PUT requests from the frontend. This allows the user to unselect a file.
  6. Code refactoring for improved maintenance:

    • Make the code more modular and easier to maintain in the long term.

Testing:

  • Comprehensive testing has been performed to ensure the stability and correctness of the changes.

Notes for Reviewers:

  • Please pay special attention to the changes in field labels, constraints, and backend handling.
  • The refactoring aims to improve code maintainability without altering existing functionality.

@MartinBelthle MartinBelthle changed the title fix(xpansion): change - to _ in relative_optimality_gap parameter fix(xpansion): change - to _ in relative_optimality_gap parameter Dec 13, 2023
@MartinBelthle MartinBelthle force-pushed the fix/xpansion-relative-gap-issue branch from 12d2fb4 to 7878fac Compare December 13, 2023 18:05
@MartinBelthle MartinBelthle self-assigned this Dec 14, 2023
@MartinBelthle
Copy link
Contributor Author

MartinBelthle commented Dec 14, 2023

NB : I've requested a review from Hatim concerning the little front-end part and from Laurent for the back-end part.

Copy link
Contributor

@laurent-laporte-pro laurent-laporte-pro left a comment

Choose a reason for hiding this comment

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

According to Thomas' specification documentation, the relaxed_optimality_gap should exist in version >= 800. The default value is 1e-5.
There is a similar issue with the field relative_gap which has the default value 1e-6.

hdinia

This comment was marked as resolved.

@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/xpansion-relative-gap-issue branch from 0b51fdf to 638e046 Compare December 22, 2023 19:24
@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/xpansion-relative-gap-issue branch from 638e046 to b3ea9d7 Compare December 23, 2023 09:53
@laurent-laporte-pro laurent-laporte-pro changed the title fix(xpansion): change - to _ in relative_optimality_gap parameter fix(xpansion): update for improved parameter handling and code refactoring Dec 23, 2023
@laurent-laporte-pro
Copy link
Contributor

@skamril, Thomas B. nous a fourni des spécifications concernant le formulaire de configuration générales d'Xpansion
(le formulaire affiché dans l'onglet "PARAMÈTRES", implémenté dans le script
src/components/App/Singlestudy/explore/Xpansion/Settings/SettingsForm.tsx). En partant de ses spécifications,
j'ai réorganisé l'ordre des champs, j'ai corrigé les libellés et ajouté des contraintes pour les champs numériques.

Actuellement, ce formulaire est soumis de manière globale, ce qui correspond bien à une requête PUT. Mais je remarque
que les champs "additional-constraints" et "yearly-weights", implémenté sous la forme d'une liste déroulante à sélection
unique (c'est-à-dire : avec un <SelectSingle>), ne sont pas soumis lorsque la sélection est vide. Pour rappel, nous
avons l'option optional qui permet à l'utilisateur de sélectionner aucun fichier. Cela pose un problème, car le
backend reçoit la requête PUT avec ces champs manquants, parce que les valeurs null (None) sont ignorées en Python.

Donc, pour palier à ce problème, j'ai rendu les attributs additional_constraints et yearly_weights obligatoires
en Python et j'ai affecté la valeur "" comme valeur par défaut. Autrement dit, si le champ est absent, le backend
le considèrera comme une chaîne vide.

J'ai fait ma propre analyse de la situation : lorsque l'utilisateur sélectionne l'option "Aucun" dans la liste,
la valeur attributé au paramètre est bien "".

Cela est fait dans le script src/components/common/SelectSingle.tsx :

<MenuItem key="None" value="">
  {t("global.none")}
</MenuItem>

Ce qui est différent de null. Donc, ça me va. Mais lors de la soumission du formulaire, les valeurs vides sont
ignorées. En toute rigueur, il faudrait ignorer les valeurs null et non pas les valeurs vides.

@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/xpansion-relative-gap-issue branch from b3ea9d7 to d2b2985 Compare January 4, 2024 16:27
@laurent-laporte-pro laurent-laporte-pro merged commit e14ebc3 into dev Jan 4, 2024
4 checks passed
@laurent-laporte-pro laurent-laporte-pro deleted the fix/xpansion-relative-gap-issue branch January 4, 2024 16:37
@laurent-laporte-pro laurent-laporte-pro added this to the v2.16.2 milestone Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modification of the relaxed optimality gap does not yield the correct change in the settings.ini file
3 participants