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.

Sign up for GitHub

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

Jump to bottom

is.na(mask) - argument is not interpretable as logical #274

Closed
dorianps opened this issue Jul 20, 2019 · 23 comments
Closed

is.na(mask) - argument is not interpretable as logical #274

dorianps opened this issue Jul 20, 2019 · 23 comments

Comments

@dorianps
Copy link
Collaborator

dorianps commented Jul 20, 2019 •
edited
Loading

is.na(mask) - argument is not interpretable as logical

This error is coming up on OSX installations of ANTsR/LINDA/LESYMAP. The error shows when an antsImage is checked if is NA (the default value if the user doesn't specify the argument). There is no problem on linux, so there must be something unusual going on in Mac. Error came up in Travis tests for LESYMAP last night, but also a user had the same problem yesterday while using LINDA, issue here.

I don't have an Apple computer and I have no clue why all of sudden checking if an antsImage variable is NA has become a problem there. Any idea?
@ntustison
Copy link
Member

I don't have an answer for you but I had the same issue pop up with ANTsRNet travis builds a couple weeks ago and ended up switching to NULL usage.

@dorianps
Copy link
Collaborator Author

You had the problem on OSX as well, right?

Did you simply switch to is.null and worked?

I wonder whether this is a change of R variable policy for the latest release, I would be surprised if this was just a mac problem.

@ntustison
Copy link
Member

It was on all the travis builds which include both linux and osx. The link above points to my fix commit and, yes, that is all I did was change NA --> NULL and "is.na" --> "is.NULL" in the case of an antsImage. That was all it took to correct the issue.

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 20, 2019 via email

@dorianps
Copy link
Collaborator Author

dorianps commented Jul 20, 2019 •
edited
Loading

Makes sense the change you made. I don't remember seeing a pull request with these changes though, was there one? Should we perhaps keep a common tracker of changes to ANTsR so we can go back to read changes without scavenging code to understand what happens (I am thinking something like the NEWS file or a wiki page).

P.s. Which commit did the is.na change happen?

@dorianps
Copy link
Collaborator Author

@muschellij2 The problem is not coming from the LINDA pakcage but from abpN4() in ANTsR. That function contains is.na(mask). I ran a quick check and looks like ANTsR uses the NA strategy for default antsImage arguments frequently. I tried fixing abpN4 myself by switching NA to NULL as suggested by @ntustison but then another error appeared for antsRegistration calls (object 'maskopt' not found). I also tried deleting the whole file zzz_finite.R which you had added last month, and besides new warnings, the maskopt error appears again.

9:39 Loading template...
19:39 Skull stripping... (long process) bad det -1 v 1 u -1
 bad det -1 v 1 u -1 new 1
Error in antsRegistration(tem, img, typeofTransform = regtype, initialTransform = initafffn,  : 
  object 'maskopt' not found
In addition: Warning messages:
1: In is.na(mask) : is.na() applied to non-(list or vector) of type 'S4'
2: In is.na(mask) : is.na() applied to non-(list or vector) of type 'S4'
Called from: antsRegistration(tem, img, typeofTransform = regtype, initialTransform = initafffn, 
    mask = temregmask, verbose = verbose)

You know better what changes you made these last couple of months. Can you please apply a fix to make things work at least in the short while. That is, either roll back the changes to allow the LINDA users get going, or edit all the functions that use is.na in ANTsR.

@dorianps
Copy link
Collaborator Author

This is the temporary solution, using a June 13 commit for ANTsRCore:

git clone https://github.com/ANTsX/ANTsRCore.git
cd ANTsRCore
git checkout 25f0f8a469950f3c66ece4c2632bb15d30cea8de
R CMD INSTALL .

@dorianps dorianps mentioned this issue Jul 22, 2019
Closed
@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 22, 2019 via email

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 22, 2019 via email

@dorianps
Copy link
Collaborator Author

@muschellij2 Not sure I understand. The current ANTsRCore is not compatible with ANTsR, because certain functions, like abpN4() fail to work. This makes any program that relies on these functions unusable, example being LINDA. Maybe it makes sense to roll back the changes you applied since June and work them in an experimental branch until there are no problems.

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 23, 2019 via email

@dorianps
Copy link
Collaborator Author

@muschellij2 Travis tests do not test for all combinations of arguments a user can put. Here is a reproducible error:

> img <- antsImageRead(getANTsRData("r16"))
> mask = getMask(img)
> temp = abpN4(img, mask=mask)
Error in if (is.na(mask)) { : argument is not interpretable as logical

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 24, 2019 via email

@dorianps
Copy link
Collaborator Author

dorianps commented Jul 24, 2019 •
edited
Loading

Tried uninstalling everything (ITKR/ANTsRCore/ANTsR) and install again with just

devtools::install_github('ANTsX/ANTsR')

The error above is the same, meaning that the devtools install method has the same problem (I was using R CMD INSTALL before)

@stnava
Copy link
Member

stnava commented Jul 24, 2019 via email

@dorianps
Copy link
Collaborator Author

Looks like that pull request addresses is.na in many functions, and also passes travis. Is there a reason to hold the merge for now? Knowing, of course, that ANTsRCore and ANTsR are not 100% compatible currently.

@dorianps
Copy link
Collaborator Author

dorianps commented Jul 29, 2019 •
edited
Loading

Pinging this issue again. @stnava is there an issue with merging the above pull request? If the merge is not happening soon, maybe ANTsRCore should reverse to an older commit to re-establish the compatibility with ANTsR.

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 29, 2019 via email

@dorianps
Copy link
Collaborator Author

dorianps commented Jul 31, 2019 •
edited
Loading

Thanks for merging. Looks like things are working for now.

A small note. Doing is.na(c(mask)) returns still a logical, which can be helpful if we want to overcome the is.na(mask) method of ANTsR which returns and antsImage.

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 31, 2019 via email

@dorianps
Copy link
Collaborator Author

Doing is.na(c(mask)) produces a single logical. That is the strategy I decided to use to correct my code instead of going for is.null()

@muschellij2
Copy link
Collaborator

muschellij2 commented Jul 31, 2019 via email

@dorianps
Copy link
Collaborator Author

Hmmm, I thought future dev might change this behavior too. Will take care of it once this happens. Please add this info to the commits/pulls that will carry the change.

Thanks, closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@stnava @ntustison @muschellij2 @dorianps