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.

Sign up for GitHub

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

Jump to bottom

Ideas for future improvements to the code (mostly a reminder to ourselves) #22

Open
7 of 21 tasks
inversepixel opened this issue Jan 17, 2024 · 4 comments
Open
7 of 21 tasks

Ideas for future improvements to the code (mostly a reminder to ourselves) #22

inversepixel opened this issue Jan 17, 2024 · 4 comments

Comments

@inversepixel
Copy link
Collaborator

inversepixel commented Jan 17, 2024 •
edited by pandersson94
Loading

Code:

  • Renaming of, e.g., float2color3() → floatToColor3() etc, because in this case it is a bit confusing. When doing this, we should rename so no function uses a 2 instead of To
  • Fix constructor for FLIP::filename("tmp.png"); so that we do not need tmp but can set the extension (png).
  • Make solve2degree() a bit more robust by using a better 2nd-degree solver.
  • Remove image() constructors that are not needed?
  • Rename saveHDROutputLDRImages() to something better, perhaps saveIntermediateLDRImages(). Same goes for corresponding variable names: returnLDRFLIPImages, hdrOutputFlipLDRImages, returnLDRImages, and hdrOutputLDRImages.
  • Use new tinyexr and stb_image_write. Or even better: git submodule?

Functionality/testing/error handling:

  • Investigate two images from Tomas. They give NaN?
  • Better handling of incorrect filenames. What happens if the file does not exist?
  • Overlapping histograms for C++ is missing.
  • Better handling of num channels and dimensions (if they differ). Do it in a single place for pyhton/cpp. If file does not exist, then we should never write any image.
  • Test if LDR images are in [0,1] and return warning or bool or something.
  • Add more test/ref images + test of histograms + test of multiple test images + test of grayscale output.
  • Handle the case when the median = 0
  • What if the API is used with different resolutions? We should return with an error in those cases.
  • Exit with error message if image load fails in Python. Currently just gives 0 error.

Speed:

  • Make it possible for the Python version to run on the GPU.
  • Make a fast CUDA version of std::nth_element() (would make the HDR-version faster). Or use thrust::sort().
  • Do profile runs on C++ and CUDA (optimize, e.g., in HDR, we have expose(), tonemap(), clamp() which can be optimized into one function).

Simplicity:

  • Publish to PyPI
  • Save a flip.exe in git-repo with hash to make it simpler to try. Perhaps only the CPU version to start with?
  • Online FLIP would be cool.
@pandersson94 pandersson94 mentioned this issue Apr 8, 2024
Merged
pandersson94 pushed a commit that referenced this issue Apr 9, 2024
Final commit for v1.4
0349494 
- Changed the Python version of ꟻLIP so that it leverages the C++ code through [pybind11](https://github.com/pybind/pybind11).
	- Results (only evaluation, not including file load/save, etc; measured on an AMD Ryzen Threadripper 3970X 32-Core Processor, 3693 MHz, with 32 Cores and 64 Logical Processors):
		- 20-47x faster for LDR/HDR CPU.
		- Timings for 1920x1080 images:
			- Python/LDR: 77 ms
			- Python/HDR: 1007 ms
	- **NOTE**: The Python version can currently _not_ run the CUDA version of ꟻLIP (see issue [#22](#22)).
	- **NOTE**: The Python tool now uses the C++ tool. Compared to before, you will need to change `_` to `-` when calling flip.py (e.g., `python flip.py -r reference.exr -t test.exr --start_exposure 3` is now `python flip.py -r reference.exr -t test.exr --start-exposure 3`; see `python flip.py -h`).
- The Python version of ꟻLIP can now be installed using `pip` (run `pip install -r requirements.txt .` from the `python` folder).
- The code for the C++/CUDA tool is now in `FLIPToolHelpers.h`.
- **NOTE**: The fourth `evaluate()` function in `FLIP.h` now takes two additional arguments: `computeMeanError` and `meanError`. Furthermore, its list of arguments has been partly reordered.
- **NOTE**: The median computation (used for automatic start and stop expsoure computations in HDR-ꟻLIP) in the C++/CUDA code has been changed, sometimes causing a minor change in results but always resulting in a significant speedup. The tests have been updated following this change.
  - Timings for 1920x1080 images (only evaluation, not including file load/save, etc, *but* measured with another GPU and including more code than the numbers presented in the v1.2 update, so the numbers are not directly comparable; measured on an AMD Ryzen Threadripper 3970X 32-Core Processor, 3693 MHz, with 32 Cores and 64 Logical Processors and an NVIDIA RTX 4090 GPU):
    - CPP/LDR: 86 ms
    - CPP/HDR: 1179 ms
    - CUDA/LDR: 8 ms
    - CUDA/HDR: 131 ms
- Added check for OpenMP for CMake build.
- Overlapped histograms are now available in the C++ tool code. These are created when one reference and _two_ test images are input, together with the `--histogram` flag.
- Text file output are now available in the C++ tool code. These are created when the `--textfile` flag is input.
- The Python and C++ tests now use the same targets.
@NVlabs NVlabs deleted a comment from pandersson94 Apr 9, 2024
@pandersson94 pandersson94 changed the title Ideas for minor improvements of the code (mostly a reminder to ourselves) Future improvements of the code (mostly a reminder to ourselves) Apr 9, 2024
@pandersson94 pandersson94 changed the title Future improvements of the code (mostly a reminder to ourselves) Ideas for future improvements to the code (mostly a reminder to ourselves) Apr 9, 2024
@Latios96
Copy link
Contributor

Hi,

are there currently any efforts to publish flip to PyPI? Or is it really just a reminder? I think having flip on PyPi would be really useful to a lot of people.

@inversepixel
Copy link
Collaborator Author

Not at the moment, but we will get back to this topic! However, it will most likely be in the fall of 2024.

@pandersson94
Copy link
Collaborator

@Latios96 , we have finally published FLIP to PyPI (see the version list). We hope you'll find that it installs and runs smoothly.

Thanks for letting us know that this would be an appreciated effort!

@Latios96
Copy link
Contributor

@pandersson94 thats great to hear, thanks for letting me now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@pandersson94 @inversepixel @Latios96