-
Notifications
You must be signed in to change notification settings - Fork 67
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
multisource #39
Comments
I would leave multisource inversion routines for cases which rely explicitely on multisource data. |
I know that adapting all those functions is a long term objective. I just wanted to point out that a lot of them will fail with multisource without explaining why, and it should be a good idea to at least warn the user that the functions cannot handle multisources. For the VBA_Iphi functions, I would however immediatly fuse VBA_Iphi, VBA_Iphi_binomial and VBA_Iphi_extended as they return exactly the same results as the latter can handles the first two cases. The main reason is that all those functions are starting to diverge (typo corrections, reordering of blocs of code) and the more we wait, the more difficult the merge. You can have a look at the branch "fix-binomial" to see my intention. And there is nothing to loose. We will keep all functionalities, and have fewer lines of code to maintain... |
I disagree about fusing these things. This is because we do not know whether later changes may not break e.g., stochastic inversion, which also uses VBA_IPhi... |
Ok, I will revert the changes in the "fix-binomial" branch. A minima, I would prepare the removal of the options.binomial field in favour of the source structure (as in "fix-binomial"). Code switches are sometimes depending on tests on sources, sometimes on binomial flag, and guessing which core function is called in which case is rather tricky. I understand that setting the binomial flag is simpler, but we could use a dedicated function as syntactic sugar for source setting, something like options = set_observation_distribution(options, 'gaussian') |
I am closing this issue follwing the merge of the pull request #45 that handle the problems related to the binomial flag. I will open another issue to reboot the discussion about the simplification of the GN tree. |
Some functions of the toolbox are not compatible with multisource and will fail, bug, or display inconsistent results if called on multisource models, often because if only relies on the options.binomial flag instead of using the options.source structure:
I started a branch fix-binomial to tackle those issues.
Moreover, some redundancy in the code could be avoided, as multisource coded can sometimes handle subcases that are currently treated by separate function (VBA_IPhi_XXX in particular).
Also, some features are currently incompatible with multisource although there is no principled reason for this except for the time necessary for implementing them in multisource, namely the one associated with the flag options.UNL and options.nmog (not mentioning stochastic inversion).
The text was updated successfully, but these errors were encountered: