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]

cpp17::optional

[patrikhuber]

For now we want to stick with eos's cpp17::nullopt for these. I think for one of the next releases, I'd like to move to C++17 as a minimum, then we can drop all the std/cpp17 "magic" for optional, but for now, let's keep it compatible with C++14.

[omar-h-omar]

Oh I see I'll make the change now

[patrikhuber]

I think we probably want to pass the landmark_definitions as a reference too, so just add const ... &.

[omar-h-omar]

Out of curiosity what is the benefit behind having it as a reference here?

[patrikhuber]

That's a good question and I was actually trying to google it before making my earlier comment. If std::optional is a "pointer type" (i.e. it holds a pointer to the underlying data, and when you pass it around, even by value, only the address/pointer is passed around), then we might as well pass it by value (without the const ... &), as it will then make no difference whether we pass around a pointer (32/64 bit) or a reference (32/64 bit). However if all the data is stored in the optional object directly, then we might potentially copy around a big/heavy object if we pass by value (without the const &).
I think optional would be a pointer type... but I couldn't really find out. So to be safe I'd just pass it by const&, even if perhaps not needed, I don't think it has a drawback either. If you know more than me, I'm happy to leave it! :-D

[omar-h-omar]

Thanks for the explanation, I can see where the issue is. Using const& makes sure we avoid passing the whole object if optional isn't a pointer. It's really interesting how C++ handles these situations. I haven't encountered anything like it in other languages.

[patrikhuber]

Yep, that's exactly it. I think it's quite cool how C++ allows you to make these choice, as it's useful if performance is relevant. And I think this is a function where we definitely don't want to pass any heavy objects (though, as I said, assuming optional is a pointer type, which I think it is, passing by value would be equally fine here).
I think in general, passing by value is a good default too, and if I remember correctly, it often allows better optimisations by the compiler. It can get quite messy too though - see e.g. https://twitter.com/herbsutter/status/1422644603883192320/photo/1. I think there's a couple of talks from Herb Sutter where he explains this, but I don't remember which one exactly.

[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]

@omar-h-omar One more std:: here.

[patrikhuber]

I think I'd also add a const here.

[patrikhuber]

Another std here, plus can add const.

[omar-h-omar]

Whoops sorry missed those

[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 :)