search

imsweb / mph

Java implementation of the Multiple Primary and Histology Coding Rules.
Other
4 stars 2 forks source link

Expose group names (#98) #101

Closed bekeles closed 1 year ago

bekeles commented 1 year ago

closes #98

bekeles commented 1 year ago

@depryf feel free to change the change log message.

depryf commented 1 year ago

I am a bit confused by the changes.

The request was to expose the names in the output class, not to remove the IDs.

IDs are the way the groups are uniquely identified, and that shouldn't change.

I see here you change every rule to pass the group name to the constructor instead of the group ID. I think we need both. But maybe the constructor should take the group object as a param, that might be more flexible.

At the end, the most important is that the MphOutput class has the ID and the name. The ID is very useful to get the full group since MphUtils has a "getGroupById" method. The name is useful to be displayed in GUI without having to constantly convert the ID to the name.

bekeles commented 1 year ago

This comment of yours made me think you don't want the IDs to be exposed. Ideally the IDs should not be exposed anywhere (but they are the internal identifiers of the groups in the code, so they are needed). I added that back.

The only reason we were passing the id to each rule was for questionable results reason. Since we are using the names in the reason, I thought we didn't need the ID anymore.

depryf commented 1 year ago

I am sorry that my comment was not clear. I meant that the IDs should not be exposed to users in our GUIs. But it's OK to expose them to the caller of the library.

There is a "MphUtils.getAllGroups" that returns a map of groups keyed by their IDs. If I call the library and it returns an output and I want to access the group that created that output, how am I supposed to get the group without the ID? I would have to iterate over the all the groups and find the one with the returned name. That just doesn't make sense IMO.

I think having the ID on the output made sense before and still make sense. And I certainly didn't mean it had to be removed from there. Sorry if that wasn't clear.

bekeles commented 1 year ago

Great! Do you still need the group id (or the Group object) to be passed to each rule? I am not seeing any use of that

depryf commented 1 year ago

Nope. I thought you would need that to have both the ID and name available in the result. But if that's not the case, we don't need to do that at all. My only request is to have ID and name on the output.

bekeles commented 1 year ago

Ok that is done and I changed one of the test cases to check both name and id are returned.