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.

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

updated lidar_hd_pre_transform function #118

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liubigli-tcp
Copy link

Hello!

Thank you very much for publishing this framework! It is really amazing!

I have created this pull request to allow any user to work with point clouds that have custom sets of features.
With this commit, the user specifies the list of point cloud features in its dataset_description yaml file.
Let assume that the point clouds in your dataset don't have any color information, the user can simply pass an empty
list in the color_keys and updates the number of input features (d_in) and that's all.

Please let me know if this could be of any interest to you :)

…oint clouds that have custom sets of features
@leavauchier
Copy link
Collaborator

Hi @liubigli-tcp
Thanks a lot for your contribution!
It looks interesting to be more flexible on the input file (especially for people who have the possibility to train a new model).
I'll see with the rest of the team to have it intergrated soon

@leavauchier
Copy link
Collaborator

Hi @liubigli-tcp,
after a second thought, I don't think modifying lidar_hd_pre_transform is the right thing to do:
The architecture of the code was made so that you can create a pretransform function which is specific to your data (lidar_hd_pre_transform correspond to the pre-transformation for the LidarHD French lidar data program) and then tell myria3d which pre-transform method to use for your data in the configuration files here:

- "${get_method:myria3d.pctl.points_pre_transform.lidar_hd.lidar_hd_pre_transform}"

More information on that in the documentation: https://ignf.github.io/myria3d/tutorials/prepare_dataset.html

However, creating a generic pre_transform could be possible (but without adding new arguments to the method that would be used as a points_pre_transform.

@liubigli-tcp
Copy link
Author

liubigli-tcp commented Oct 14, 2024

Hi @leavauchier,

Thank you so much for your response! I really appreciate it. I’m currently working with point clouds that differ slightly from the LidarHD French lidar program, and I was wondering if there’s a way to adapt the code for other types of point clouds without having to write a new pre-transform function each time—just by adjusting the config files. This way, I would only need a simple system to manage my set of config files.

From what you’ve explained, it seems my approach might diverge too much from the way you originally designed the framework. For example, I would still need to add a custom function for each new type of point cloud.

If I also need to maintain the code for implementing my custom pre-transform functions, do you think it would make more sense for me to fork your repository and make these changes in my own version?

@CharlesGaydon CharlesGaydon requested review from leavauchier and removed request for leavauchier October 31, 2024 18:11
@leavauchier
Copy link
Collaborator

Hi @liubigli-tcp,
the idea was indeed to have a pre-transform for each kind of point cloud, and I think we are not ready to re-think the way it works now (because it would imply too much reworks on this code that we are maintaining more than developing actively at the moment).

Maybe there can be a middle ground to find by creating an additional pre-transform which is more flexible in addition to the existing one.It will require a way to have something generic using the config but without adding new arguments to the method that would be used as a points_pre_transform in order not to interfere with the existing pre-transforms.

In this case, if it does not affect the existing workflow, we would be glad to add it to our codebase. Otherwise, I think it would be easier to keep your own for such developments, or in the case you want to add many specific use cases.

@liubigli-tcp
Copy link
Author

Hi @leavauchier,

Thank you for your kind reply! Regarding the "middle ground" you mentioned, what do you think about renaming my pre transform function to something like base_pre_transform, while keeping its signature and implementation as I have proposed? This function would be added alongside the existing lidar_hd_pre_transform.

This solution is fully backward-compatible with the current implementation and does not require any changes for existing users of the library. At the same time, it provides flexibility for anyone working with other types of point clouds, as they can adapt the configuration file to suit their custom needs.

Please let me know if this solution aligns better with your requirements. I’d be more than happy to make the necessary changes to integrate this function into your framework! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants