Modifying the landmark mapper loop to support hybrid mappings

[omar-h-omar]

@patrikhuber I've modified one of the fitting loops to allow for "hybrid" mappers where some points are mapped from landmark identifier to landmark definitions and others are mapped directly. If you're happy with the change below, I will apply those changes to the other fitting functions. I was actually thinking of modifying the function get_corresponding_pointset which seems to contain a simple landmark conversion loop and is currently not being used. What do you think?

[patrikhuber]

Hi @omar-h-omar,

Well spotted with the function get_corresponding_pointset(). Indeed I've added this long ago (probably exactly because this conversion loop appears in multiple places), but then I didn't seem to have used it anywhere.

I was just thinking how to best do this, as it would indeed be nice to "revive" this function so we don't have this loop duplicated in several places. I don't think it's trivial: Sometimes we want the function to just return image_points, sometimes we need model_points too, and sometimes these come from the mean shape, but sometimes we have an existing Mesh instance to take the model_points from. get_corresponding_pointset() has an interesting "note" in its comments, it suggests to split the function into Landmarks => vertex IDs and vertex IDs => model_points. Maybe that's the way to do it: We modify get_corresponding_pointset() so it takes a LandmarkCollection, and outputs image_points and vertex_ids. Then the caller handles creating model_points, if they need it. We could even slim it down further and just take one Landmark, then the caller has to provide the loop if they need one and has to also deal with what to do if a mapping is missing, which makes more sense and doesn't hide it if this happens by "user mistake". Or even better, maybe LandmarkMapper::convert() should get an extra argument optional<LandmarkDefinitions> and then the mapping (for just one landmark) happens in there? I think perhaps this would make most sense, as the LandmarkMapper is already sort of the "interface" point between a Landmark and MorphableModel? As it is now, the user has to call LandmarkMapper::convert() and then also use the LandmarkDefinitions... which is a bit confusing. So LandmarkMapper::convert() could just encapsulate that.

Let me know what you think - I think I might be leaning towards my last idea.

On another note, in your loop, there's if (morphable_model.get_landmark_definitions()) but it doesn't have an else - what happens in that case?

[patrikhuber]

Thinking about this some more, maybe putting it all in LandmarkMapper::convert() isn't the best idea. For, say, an ibug landmark "13", we do want that function to return "eye.right.outer" (just a made-up example), and not directly the vertex ID. So perhaps we want a function get_vertex_index() that takes a Landmark, LandmarkMapper, and optional LandmarkDefinitions. This function then calls LandmarkMapper::convert() and contains your new logic. What do you think of that?

It also quite clearly expresses what we actually want - get the vertex index, either directly or indirectly, through whatever means needed (the caller doesn't care about that - they just want the vertex index).

[omar-h-omar]

Hi @patrikhuber,

Actually my initial approach was to modify the LandmarkMapper::convert(). Though after seeing the function, it seemed like we want to return the landmark definition sometimes like "eye.right.outer" as you said.

I think creating get_vertex_index() is the best approach here. Then the caller can handle getting the model_points on their own. I'm gonna give it a try on my end!

About the landmark conversion loop, I was unsure if we want to raise an exception if the user didn't map the landmarks directly and no landmark definitions were found? Currently if (morphable_model.get_landmark_definitions()) will check if the landmark definitions do exist. Otherwise it will go out of the if statement and continue to the next landmark, essentially ignoring the landmark. Personally, I'm leaning towards being strict about this and raising an exception to avoid users mapping landmarks incorrectly. What do you think?

[omar-h-omar]

I've added get_vertex_index and removed the unused get_corresponding_pointset. I've tested it on my end and it works!

[patrikhuber]

I think this looks great, thank you Omar!

I was unsure if we want to raise an exception if the user didn't map the landmarks directly and no landmark definitions were found? Currently if (morphable_model.get_landmark_definitions()) will check if the landmark definitions do exist. Otherwise it will go out of the if statement and continue to the next landmark, essentially ignoring the landmark. Personally, I'm leaning towards being strict about this and raising an exception to avoid users mapping landmarks incorrectly. What do you think?

Good question. So if I understand correctly, this would happen if the user uses a landmarking scheme where say a landmark_name would be "nose.outer_corner" or something, but then a particular model's landmark_definitions wouldn't contain that specific landmark. Is that correct? I think I'd lean towards also just returning a nullopt for this case. I think there could be a plausible case where the user just wants to skip over these landmarks, if they exist? And we do tell them with the nullopt that some skipping is happening, so if a user is worried about skipping landmarks, they could check / handle it themselves? What do you think?

[omar-h-omar]

I think this looks great, thank you Omar!

I was unsure if we want to raise an exception if the user didn't map the landmarks directly and no landmark definitions were found? Currently if (morphable_model.get_landmark_definitions()) will check if the landmark definitions do exist. Otherwise it will go out of the if statement and continue to the next landmark, essentially ignoring the landmark. Personally, I'm leaning towards being strict about this and raising an exception to avoid users mapping landmarks incorrectly. What do you think?

Good question. So if I understand correctly, this would happen if the user uses a landmarking scheme where say a landmark_name would be "nose.outer_corner" or something, but then a particular model's landmark_definitions wouldn't contain that specific landmark. Is that correct? I think I'd lean towards also just returning a nullopt for this case. I think there could be a plausible case where the user just wants to skip over these landmarks, if they exist? And we do tell them with the nullopt that some skipping is happening, so if a user is worried about skipping landmarks, they could check / handle it themselves? What do you think?

Yes that's exactly the scenario I was thinking of. My main worry was just that some users may not be aware of the skipping. I can see how the skipping can be useful as well. How would you feel about raising a runtime warning before returning a nullopt just to say "{{Lanmark_name}} could not be mapped to a vertex index. This landmark will be not be used during fitting."? It's a minor tweak but might be helpful.

[patrikhuber]

Yes that's exactly the scenario I was thinking of. My main worry was just that some users may not be aware of the skipping. I can see how the skipping can be useful as well. How would you feel about raising a runtime warning before returning a nullopt just to say "{{Lanmark_name}} could not be mapped to a vertex index. This landmark will be not be used during fitting."? It's a minor tweak but might be helpful.

How could a user not be aware of the skipping though, since we return an optional in get_vertex_index()?

If I understand you correctly, you'd propose a throw std::runtime_exception (or something like that)? But then, in theory, every user would have to wrap the call in try/catch, and if a user wants to genuinely skip these landmarks, they'd have to handle it in their catch ... path. I think the second case is definitely not how exceptions are meant to be used - as far as I know, the catch-path can incur quite significant runtime costs (I think it causes stack-unwinding and all sorts of things), which is not what you'd want to do in a fitting loop. So I'm really leaning towards just returning a nullopt there is probably best. Or did you mean anything else by raising a "runtime warning"?

[omar-h-omar]

Yes that's exactly the scenario I was thinking of. My main worry was just that some users may not be aware of the skipping. I can see how the skipping can be useful as well. How would you feel about raising a runtime warning before returning a nullopt just to say "{{Lanmark_name}} could not be mapped to a vertex index. This landmark will be not be used during fitting."? It's a minor tweak but might be helpful.

How could a user not be aware of the skipping though, since we return an optional in get_vertex_index()?

If I understand you correctly, you'd propose a throw std::runtime_exception (or something like that)? But then, in theory, every user would have to wrap the call in try/catch, and if a user wants to genuinely skip these landmarks, they'd have to handle it in their catch ... path. I think the second case is definitely not how exceptions are meant to be used - as far as I know, the catch-path can incur quite significant runtime costs (I think it causes stack-unwinding and all sorts of things), which is not what you'd want to do in a fitting loop. So I'm really leaning towards just returning a nullopt there is probably best. Or did you mean anything else by raising a "runtime warning"?

I was actually thinking of users using eos from python. They may not be aware of the skipping since fit_shape_and_pose doesn't mention it in its brief. Ofc if they look at the code it's quite clear that unmapped landmarks are skipped.

For the warning, I was actually thinking of raising it similar to how it's done in python. However after some googling, it looks like that isn't an option in C++. So in this case, you're right. Let's just return a nullopt

[patrikhuber]

Good point with calling it from Python. Indeed it could get unnoticed in that scenario, and probably cause a lengthy debugging session for someone. But I don't have a good idea how we could make it better, except for just making sure the documentation is good. Indeed C++ doesn't have any mechanism for warnings.

[patrikhuber]

Looks great, thank you! Merging.

[omar-h-omar]

Good point with calling it from Python. Indeed it could get unnoticed in that scenario, and probably cause a lengthy debugging session for someone. But I don't have a good idea how we could make it better, except for just making sure the documentation is good. Indeed C++ doesn't have any mechanism for warnings.

Yeah It's a shame. I don't know how to make it better as well though. Hopefully, the documentation should be enough :)