Skip to content

Conversation

@HenkMutsaerts
Copy link
Member

Linked issue

Closes #892

How to test

Required: if not defined in the linked issue, add a simple test description here

Comments

Optional: add helpful comments for the reviewers here

Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

OK

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Nov 11, 2021

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.

Edit: please add the "how to test" section next time, we kind of agreed on this.

@MichaelStritt MichaelStritt changed the title #892 ExploreASL_Initialize: rm paths before addpath Closes: #892 ExploreASL_Initialize: rm paths before addpath Nov 11, 2021
@MichaelStritt MichaelStritt changed the title Closes: #892 ExploreASL_Initialize: rm paths before addpath Closes #892 ExploreASL_Initialize: rm paths before addpath Nov 11, 2021
@MichaelStritt MichaelStritt added the feature New feature, enhancement or request label Nov 11, 2021
@HenkMutsaerts
Copy link
Member Author

HenkMutsaerts commented Nov 12, 2021

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.

Edit: please add the "how to test" section next time, we kind of agreed on this.

  • Thanks, but we have to remove the Matlab toolbox condition. There are users having e.g., SPM in their matlab toolbox, this is what we want to remove from paths. This is why I had a filesep in front of the regular expressions, so it wouldn't detect /alst but it would detect /lst

  • We also have to remove the version related regex, why is this there? Again, this won't cause a warning if you have multiple ExploreASL or toolbox versions per se. This will only cause a warning if you have initialized (i.e., added to the Matlab path) multiple versions; which is actually what we want to rule out.

  • I disagree with the verbose mode, or we have to default it to true. Users should not have initialized other spm versions, cat12 versions, or lst versions, this should always give a warning. We could reduce the number of warnings to max 1 per toolbox.

  • Note that this isn't a feature, but a bug. Users that have ExploreASL-specific toolboxes installed also in Matlab/Toolbox — such as Wibeke had — will get strange errors when running SPM functions inside ExploreASL, related to the version differences between our SPM version and the version installed outside of ExploreASL

@HenkMutsaerts HenkMutsaerts added bug Something isn't working and removed feature New feature, enhancement or request labels Nov 12, 2021
@MichaelStritt
Copy link
Contributor

MichaelStritt commented Nov 12, 2021

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.
Edit: please add the "how to test" section next time, we kind of agreed on this.

* Thanks, but we have to remove the Matlab toolbox condition. There are users having e.g., SPM in their matlab toolbox, this is what we want to remove from paths. This is why I had a filesep in front of the regular expressions, so it wouldn't detect `/alst` but it would detect `/lst`

* We also have to remove the `version` related regex, why is this there? Again, this won't cause a warning if you have multiple ExploreASL or toolbox versions per se. This will only cause a warning if you have initialized (i.e., added to the Matlab path) multiple versions; which is actually what we want to rule out.

* I disagree with the verbose mode, or we have to default it to true. Users should not have initialized other spm versions, cat12 versions, or lst versions, this should always give a warning. We could reduce the number of warnings to max 1 per toolbox.

* Note that this isn't a feature, but a bug. Users that have ExploreASL-specific toolboxes installed also in `Matlab/Toolbox` — such as Wibeke had — will get strange errors when running SPM functions inside ExploreASL, related to the version differences between our SPM version and the version installed outside of ExploreASL

@HenkMutsaerts & @jan-petr: For me all of this is asking for trouble. I got a ton of warnings and some errors afterwards just during coding, because Matlab was complaining about missing paths. I definitely wouldn't merge it like this. If a user decides to put spm inside the internal matlab directory, something is wrong on their side on not in ExploreASL.

Especially if multiple users share a license they get like 100 warnings every time they initialize. That's crazy. Maybe you don't see it on your side, but that's not tested properly.

@HenkMutsaerts
Copy link
Member Author

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.
Edit: please add the "how to test" section next time, we kind of agreed on this.

* Thanks, but we have to remove the Matlab toolbox condition. There are users having e.g., SPM in their matlab toolbox, this is what we want to remove from paths. This is why I had a filesep in front of the regular expressions, so it wouldn't detect `/alst` but it would detect `/lst`

* We also have to remove the `version` related regex, why is this there? Again, this won't cause a warning if you have multiple ExploreASL or toolbox versions per se. This will only cause a warning if you have initialized (i.e., added to the Matlab path) multiple versions; which is actually what we want to rule out.

* I disagree with the verbose mode, or we have to default it to true. Users should not have initialized other spm versions, cat12 versions, or lst versions, this should always give a warning. We could reduce the number of warnings to max 1 per toolbox.

* Note that this isn't a feature, but a bug. Users that have ExploreASL-specific toolboxes installed also in `Matlab/Toolbox` — such as Wibeke had — will get strange errors when running SPM functions inside ExploreASL, related to the version differences between our SPM version and the version installed outside of ExploreASL

@HenkMutsaerts & @jan-petr: For me all of this is asking for trouble. I got a ton of warnings and some errors afterwards just during coding, because Matlab was complaining about missing paths. I definitely wouldn't merge it like this. If a user decides to put spm inside the internal matlab directory, something is wrong on their side on not in ExploreASL.

Especially if multiple users share a license they get like 100 warnings every time they initialize. That's crazy. Maybe you don't see it on your side, but that's not tested properly.

OK then it needs to be made more stable, such that it will only throw away important paths. Or we can for now include only spm, that should be sufficient as CAT12 & LST are usually in spm paths, and fsl is dealt with separately. Let me add a change and can you test it then again Michael?

@MichaelStritt
Copy link
Contributor

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.
Edit: please add the "how to test" section next time, we kind of agreed on this.

* Thanks, but we have to remove the Matlab toolbox condition. There are users having e.g., SPM in their matlab toolbox, this is what we want to remove from paths. This is why I had a filesep in front of the regular expressions, so it wouldn't detect `/alst` but it would detect `/lst`

* We also have to remove the `version` related regex, why is this there? Again, this won't cause a warning if you have multiple ExploreASL or toolbox versions per se. This will only cause a warning if you have initialized (i.e., added to the Matlab path) multiple versions; which is actually what we want to rule out.

* I disagree with the verbose mode, or we have to default it to true. Users should not have initialized other spm versions, cat12 versions, or lst versions, this should always give a warning. We could reduce the number of warnings to max 1 per toolbox.

* Note that this isn't a feature, but a bug. Users that have ExploreASL-specific toolboxes installed also in `Matlab/Toolbox` — such as Wibeke had — will get strange errors when running SPM functions inside ExploreASL, related to the version differences between our SPM version and the version installed outside of ExploreASL

@HenkMutsaerts & @jan-petr: For me all of this is asking for trouble. I got a ton of warnings and some errors afterwards just during coding, because Matlab was complaining about missing paths. I definitely wouldn't merge it like this. If a user decides to put spm inside the internal matlab directory, something is wrong on their side on not in ExploreASL.
Especially if multiple users share a license they get like 100 warnings every time they initialize. That's crazy. Maybe you don't see it on your side, but that's not tested properly.

OK then it needs to be made more stable, such that it will only throw away important paths. Or we can for now include only spm, that should be sufficient as CAT12 & LST are usually in spm paths, and fsl is dealt with separately. Let me add a change and can you test it then again Michael?

Sure. I guess we should just change our general method. Instead of removing everything besides ExploreASL, we should remove only the things that could be troublesome, so all paths besides the ExploreASL ones that include our internal toolboxes. Meaning we only throw out other xASL, spm, bids-matlab, cat, and lst related paths, right?

@HenkMutsaerts
Copy link
Member Author

HenkMutsaerts commented Nov 13, 2021

@jan-petr & @HenkMutsaerts: did some changes to reduce the crazy verbosity, I bet users would greatly complain about that. I also added a condition for matlab related toolboxes, because that leads to super annoying bugs otherwise.
Edit: please add the "how to test" section next time, we kind of agreed on this.

* Thanks, but we have to remove the Matlab toolbox condition. There are users having e.g., SPM in their matlab toolbox, this is what we want to remove from paths. This is why I had a filesep in front of the regular expressions, so it wouldn't detect `/alst` but it would detect `/lst`

* We also have to remove the `version` related regex, why is this there? Again, this won't cause a warning if you have multiple ExploreASL or toolbox versions per se. This will only cause a warning if you have initialized (i.e., added to the Matlab path) multiple versions; which is actually what we want to rule out.

* I disagree with the verbose mode, or we have to default it to true. Users should not have initialized other spm versions, cat12 versions, or lst versions, this should always give a warning. We could reduce the number of warnings to max 1 per toolbox.

* Note that this isn't a feature, but a bug. Users that have ExploreASL-specific toolboxes installed also in `Matlab/Toolbox` — such as Wibeke had — will get strange errors when running SPM functions inside ExploreASL, related to the version differences between our SPM version and the version installed outside of ExploreASL

@HenkMutsaerts & @jan-petr: For me all of this is asking for trouble. I got a ton of warnings and some errors afterwards just during coding, because Matlab was complaining about missing paths. I definitely wouldn't merge it like this. If a user decides to put spm inside the internal matlab directory, something is wrong on their side on not in ExploreASL.
Especially if multiple users share a license they get like 100 warnings every time they initialize. That's crazy. Maybe you don't see it on your side, but that's not tested properly.

OK then it needs to be made more stable, such that it will only throw away important paths. Or we can for now include only spm, that should be sufficient as CAT12 & LST are usually in spm paths, and fsl is dealt with separately. Let me add a change and can you test it then again Michael?

Sure. I guess we should just change our general method. Instead of removing everything besides ExploreASL, we should remove only the things that could be troublesome, so all paths besides the ExploreASL ones that include our internal toolboxes. Meaning we only throw out other xASL, spm, bids-matlab, cat, and lst related paths, right?

@MichaelStritt
But this was exactly what the script did from the get-go :) It searched for folders that contained these toolboxes and only removed those paths :) Now I made the script more stable, by adding the requirement for the filesep. So it will only remove spm and bids-matlab for now.

  • I had to remove the Matlab-clause, because we also want to remove e.g., /Matlab/Toolbox/spm from the path.

  • I would keep the warning verbosity by default. Imagine the use case where the user adds a newer spm installation to the path, and assumes that ExploreASL uses it. In this case, it is useful for the user to know that we will still use our own modified spm installation.
    When the paths are removed, it won't give the warning again when rerunning ExploreASL_Initialize. If this is annoying to you, I would remove the spm paths from your Matlab installation. Alternatively, we can set it up that it will only give a warning once, rather than repeating the warning for each spm folder.

@MichaelStritt MichaelStritt merged commit f451816 into develop Nov 15, 2021
@MichaelStritt MichaelStritt deleted the bug-#892_PathConflicts branch November 15, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Existing toolbox Matlab paths conflict with ExploreASL toolboxes

4 participants