search

tmills / ctakes-docker

Apache License 2.0
23 stars 18 forks source link

readme step is confusing #12

Closed tmills closed 6 years ago

tmills commented 6 years ago

Hi Matthew (@MatthewVita), I'm a bit unclear on one of the new instructions in the README. The first step under running under collection reader is to edit docker-fast-dictionary.xml to change the import, but there is no import by default in that file, so that could be confusing. Can we try to clarify what we want there?

MatthewVita commented 6 years ago

Hi Tim,

Oops. I believe it should be https://github.com/tmills/ctakes-docker/pull/13

Do you think there is a way we can make this a relative path?

Thanks, Mattew

tmills commented 6 years ago

I guess I still don't see how that would be called. If you run with: desc/localDeploymentDescriptor.xml it calls out to remoteFull.xml (which in turn references docker-fast-dictionary.xml locally), and if you run with: desc/localDeploymentDescriptorNoDeid.xml it calls out to remoteNoDeid.xml, which already references docker-fast-dictionary.xml correctly.

I think we actually might be able to just delete remoteFastDescriptor.xml?

MatthewVita commented 6 years ago

Okay Tim,

This is one of those classic "just trying to getting it working and noting down the steps situations.... and it turns out one of the steps actually has no effect!".

I believe you are correct that remoteFastDescriptor.xml is simply not used in any meaningful way.

I will put together a PR today or tomorrow to remove the file.

UPDATE: I have a PR here: https://github.com/tmills/ctakes-docker/pull/14 it needs to be tested. Have to run to a meeting now, but I will test in full later.

Thanks, Matthew

tmills commented 6 years ago

I've definitely been there! Thanks for confirming, and let me know when you get a chance if your test works.

MatthewVita commented 6 years ago

Okay sir,

The PR is testing well without de-id. I tested with the GUI viewer as well as with the collection reader. If you could, please make sure the PR doesn't break your mist stuff: https://github.com/tmills/ctakes-docker/pull/14

Ready to go!

Thanks, Matthew