Repository navigation
Merge upstream master and adopt the upstream conventions - #7
Open
ManuelOsburg wants to merge 27 commits into
Open
ManuelOsburg wants to merge 27 commits into
ManuelOsburg wants to merge 27 commits into
Conversation
…files read_hdf only builds analysis/channel0/all_parameters.h5 from the step 3 files if the binary does not exist yet. In the test "Default Analysis Is Not Changed By The Bgsub Column" the binary already existed from the previous suites, so both analysis runs read the same binary and the stripped files were never used. Remove the binary before each run. The strip keyword now also shortens the column header and takes the number of columns to keep, so it can simulate files of any older format. (cherry picked from commit ed7ddc9)
The noise on the input extinction coefficients was drawn without seed, so the rmse checks against the ground truth varied between runs (bgsub on clean images 0.039-0.066 against the bound 0.06 in repeated runs). create_test_data and create_test_data_stacked now seed the generator. The default seed is 1: with seed 0 the nonlinear solver misses image 3 of the test data (rmse 0.178; linear solver on the same data 0.014), while seeds 1-7 are fine (nonlinear rmse <= 0.036). This is a weak case of the nonlinear solver, not of the test bounds. (squashed from 1df8435 and db59b0b on ManuelOsburg/LEDSmokeAnalysis)
An experiment can consist of thousands of photos and its analysis can run for hours, but a single unreadable photo - truncated by a full memory card, lost in a faulty transfer or never written at all - aborted the whole run with the error of the underlying imaging library, and reading an image without EXIF metadata even quit the process with exit(1). The image reading functions now raise CorruptedImageError instead, the analysis skips those images and continues, and every skipped image is listed in 'analysis/corrupted_images.csv' together with the analysis step that skipped it and the type and message of the original error, so the cause stays visible. Repeated and restarted runs do not duplicate entries. Images are skipped at the two points where they are read: - while the image metadata is collected, which also leaves them out of 'image_infos.csv' and 'image_infos_analysis.csv' and therefore out of every following step. The IDs of the readable images stay a gapless sequence, as they already do for images left out by num_skip_imgs. - while the LED intensities are extracted, which catches images whose metadata is intact but whose pixel data is not. The multicore analysis returns these errors from its worker processes and reports them in the parent, so that only one process writes the report file. Three images are indispensable, so instead of being skipped they stop the run with a message naming the file and the config key to change: the reference image of step 1, the first image of the experiment, which defines the start time, and the time reference image. A run in which no image at all could be read reports that as well instead of writing an empty analysis. The plot of the search areas is merely illustrative and is skipped with a note. Unsupported file extensions are reported the same way, which also replaces the UnboundLocalError that reading such a file used to raise. Covered by a new acceptance test suite that corrupts images in both ways and checks that the readable ones are still analysed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…column names from the column count LEDAnalysisData.__str__ concatenated the main and the fit part without a delimiter, so the last main field and led_center_x were glued into one field. Before the bgsub column this was max_col_value (an int) and the glued field still parsed as a number, with all following columns shifted by one. With bgsub_sum_col_value last, two floats in scientific notation are glued and np.loadtxt fails, so -s3 followed by --analysis crashed. Add the comma. _get_column_names extended the names by position and missed fit_time, so the check against the column count sent fitted files to _get_old_columns, which returned no names. The names are now looked up by the exact number of columns (img_id is not in the file but prepended on load, hence + 1): the -s3_fast layouts with and without bgsub and the fitted layout, all names including fit_time. Fitted files written before this fix (18 columns) are still read with the previous names and a warning that max_col_val and the fit columns are not valid; sum_col_val and mean_col_val are. The oldest layouts (4 and 15 columns) are handled as before, any other column count raises a ValueError. Unit tests cover a fitted row written by LEDAnalysisData, the legacy fitted layout, the oldest layouts and unknown column counts. Reported by Kristian Börger in FireDynamics#131.
Step 3 with fits (-s3) followed by --analysis was not covered by the acceptance tests, the corresponding S3 test is commented out. The new test in suite 07 runs -s3 on the test data and checks the extinction coefficients with sum_col_val and bgsub_sum_col_val against the ground truth. It fails on 630fcec (the binary cannot be built) and passes with the delimiter fix; -s3 takes about 2 minutes on the test data.
…ients
An image that is skipped while the LED intensities are extracted keeps its
ID in 'image_infos_analysis.csv' but writes no '{id}_led_positions.csv', so
the analysis step hit a missing file. The handler for that case was dead
code: it built its placeholder row with DataFrame.append, removed in pandas
2.0, so '-a' aborted with an AttributeError instead.
Repairing the placeholder would not have helped either, because it carried
a NaN led_array_id and a single row for a whole image, so load_img_data
filtered it out again and the NaNs would have reached the solver.
Images without results are therefore left out of the binary data entirely
and reported with their IDs, pointing at 'analysis/corrupted_images.csv'
for the ones that could not be read. Two places assumed the image IDs form
a gapless range and are fixed accordingly:
- the extinction coefficients are now paired with the experiment times of
the images they were actually computed from, instead of the first n times
of the image infos file. The time axis of the result therefore has a gap
where an image was skipped rather than a shift, and a length mismatch
raises instead of being column-stacked into silently wrong times.
- multiindex_series_to_nparray, used by the multicore analysis, indexed the
images as 1..n and now iterates over the IDs that are present.
An analysis without any results at all now reports that instead of failing
in the concat of an empty list.
Covered by a new acceptance test that corrupts an image, runs the whole
chain through '--analysis' and checks that the skipped image contributes no
row while every remaining row keeps its own experiment time.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rrupted_images Develop skip corrupted images
EXIF stores DateTimeOriginal with second resolution only, so every image time was rounded to a whole second and images taken less than a second apart shared one. Cameras keep the fraction of the second in SubSecTimeOriginal, which get_exif_datetime() now adds to the timestamp. The image times, the experiment start time and the camera clock offset keep that resolution. This makes exif_time_infront_real_time a plain float in seconds instead of the former seconds.milliseconds notation, and the postprocessing no longer rounds Experiment_Time[s] to whole seconds, which would drop images of a series and detach the LED parameters from the time axis of the extinction coefficients. Two bugs on the way: exiv2 raises Exiv2Error and not KeyError for an entry it does not have, so an image without DateTimeOriginal aborted the analysis instead of being reported and skipped, and the photo renamer left the images of a series in the arbitrary order of the directory listing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd_image_times Read the sub-second part of the EXIF capture time
Reading or writing a text file without naming an encoding uses the one of the locale, which is cp1252 on a German Windows and cannot represent the characters LEDSA writes into its configs and reports: creating the stacked analysis template failed with a UnicodeEncodeError on the box drawing characters of its section headers, and the time alignment report failed on its delta sign. Every text file is now written as utf-8 and read back explicitly, the configs users edit by hand as utf-8-sig, so a byte order mark left by a Windows editor does not end up in the name of the first option. Alongside that: - os.path.join() was called with a single concatenated argument when reading the start time, which only produced a valid path when img_directory happened to end with a separator. - File extensions are matched lowercased, as cameras and operating systems differ in the case they write them in; .nef, .arw and .dng were added to the RAW formats, which rawpy reads just like the Canon ones. - os.replace() instead of os.rename(), which does not overwrite on Windows. - Printed output stays ASCII, so a redirected stdout cannot fail to encode. - requires-python stops below 3.13, since numpy 1.26.4 has no wheels beyond CPython 3.12 and a newer interpreter would try to build it from source, which needs a compiler toolchain most Windows machines do not have. The acceptance tests now run on Windows as well, where they are allowed to fail until their first green run, and a check fails the build on any text I/O that does not name an encoding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pipeline itself ran through on the Windows job; what failed were three assumptions of the test harness: - The suite teardown removed the working directory while the process was still in it, which Windows refuses with WinError 32. That failure alone marked all twelve tests as failed, including the suites that had passed. - The number of LEDs reported by step 1 was read by slicing the last bytes of its output. That window shifts by one character per \r\n line ending, so the check compared '00 ' with 100. - The report of the corrupted images keeps the separator of the configured img_directory, which is not the one os.path.join produces, so the expected path never matched a reported one. Both are compared as paths now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The noise put on the extinction coefficients of the test images was drawn from the global numpy generator, so every run analysed different data, while the accepted RMSE of the reconstruction is compared against a fixed 0.05 - the very value the noise uses as its standard deviation. The analysis test therefore passed or failed by chance, which is how it came up red on Windows and green on Linux in the same commit. With the generator seeded, the reconstruction error is the same on every run and platform: the four RMSEs of the linear solver are 1e-10, 0.014, 0.016 and 0.022 against the threshold of 0.05. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_compatibility make the data extraction and analysis run on Windows
The Windows leg of the acceptance tests was allowed to fail while the port was still being confirmed. It passed in PR FireDynamics#139, so the exemption is no longer needed and a regression on Windows should be as visible as one on Linux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_windows_tests Let a failure of the Windows acceptance tests fail the workflow
The extinction coefficient files carry their own time axis, while the LED parameters are put on the times read from the image infos. Re-running step 1 or step 3 therefore moves the LED parameters but leaves the coefficients on the times of the earlier run, and nothing noticed: both data frames were simply indexed by times that no longer belonged to the same images. Reading them now warns, naming the file and the first experiment time the image infos do not contain. Images the analysis left out cover fewer times than the image infos without anything being out of sync, so only times without a counterpart are reported. Both cases are covered by the acceptance tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ime_axis_check Report extinction coefficients left over from an earlier run
…ature/local-background-subtraction Conflict in LedsaATestLibrary.py: keep master's NOISE_SEED with np.random.default_rng and drop the seed parameter of this branch; keep the background argument of create_test_data, create_test_image and create_img_array and the reference_property argument of load_extinction_coefficients_computed; keep both the keywords of this branch and the new ones of master. The strip keyword now names the encoding of its text I/O, as required by the encoding check of the CI. The background subtraction suite becomes 08_test_background_subtraction, since master now has 07_test_corrupted_images.
…tests Required by the encoding check of the CI (master since FireDynamics#139): two write_text calls in test_background_subtraction.py did not name one.
…imiter Fitted files written by this branch before the delimiter fix (19 columns, bgsub_sum_col_value glued to led_center_x) cannot be loaded at all: np.loadtxt fails before any column name is derived, and with silent=True only the bare ValueError was shown. _get_column_names now re-raises it with the file name and the hint to run step 3 again, like the warning for the 18-column fitted files of master. Suggested by Kristian Börger in FireDynamics#131.
The new suite 07 (corrupted images) runs before it and empties analysis/channel0 in its teardown, so the first tests of the background subtraction suite found no step 3 files. A suite setup now runs -s3_fast in the work directory and removes a binary of an earlier run. With master's NOISE_SEED the rmse checks of the suite keep their bounds with margin: bgsub on clean images 0.045 / 0.034 / 0.029 (< 0.06), with background sum 0.13 / 0.18 / 0.13 (> 0.05) and bgsub 0.014 / 0.016 / 0.022 (< 0.05), fitted output sum 0.014-0.022 (< 0.05) and bgsub 0.029-0.045 (< 0.06).
The config documents a format spec such as img{:04d}.jpg for the zero
padded IDs most cameras write, and init_functions and DataExtractor cast
the ID to int before formatting. get_start_time and
in_time_diff_to_img_time passed the ID as read from config.ini, a
string, so with a format spec they failed with "ValueError: Unknown
format code 'd' for object of type 'str'" - get_start_time whenever the
start time is taken from the first image, which is the default. Cast
the ID there as well.
New acceptance suite 09 runs the workflow with zero padded test images
and img_name_string = test_img_{:04d}.jpg, once with the start time from
the first image and once with the clock offset from a time reference
image given by its ID. Without the fix both stop with the ValueError
above.
…ackground-subtraction Feature/local background subtraction
…rmat-in-config Cast the image ID in ConfigData before formatting the image name
Since FireDynamics#137 an image that cannot be read in step 3 keeps its ID in 'image_infos_analysis.csv' but writes no '{id}_led_positions.csv', so it is missing from 'all_parameters.h5'. The single camera analysis handles that since 4bc6723, the stacked analysis did not: - _load_time_map built the time map from every row of the image infos file, keyed by the row position, and therefore contained the IDs of images without LED data. - _compute_time_alignment intersected those times, so a common time could have no image with data in one of the simulations. - _iter_aligned_images then ran into a KeyError on calculated_img_data, warned about the missing img_id and dropped the time step. - save column-stacked all common times with the fewer computed coefficients and failed in np.column_stack. The time map is now keyed by the '#ID' column and holds only the images the sub-solver actually loaded intensities for. A time at which one simulation has no data therefore drops out of the common time axis, unless another image of that simulation lies within time_sync_tolerance, and the time-alignment report counts only images with data. The length check in save is left as a guard against future regressions, raising with both counts instead of letting the column stack fail, as ExtinctionCoefficients.save does. Covered by a new acceptance test that breaks one image of the upper camera before its intensities are extracted, runs '-as' and checks that the time of that image is missing from the result while the remaining times still carry the coefficients of their own images. The test builds its own copy of the stacked test data, because steps 1 and 2 cannot be repeated in a directory they already ran in, and lowers time_sync_tolerance below the 1.25 s the test images lie apart, which otherwise lets a neighbour stand in for the skipped image. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_analysis_skipped_images leave images without extraction results out of the stacked analysis
Brings in the upstream changes since the merge of the local background subtraction: the Windows port (FireDynamics#139), corrupted image handling (FireDynamics#137), the image name fix in ConfigData (FireDynamics#142) and skipped images in the stacked analysis (FireDynamics#144). The fork features are adapted to the upstream conventions: - image names: format_img_name is dropped; the IDs are cast to int as upstream does, so zero padded names need a format spec such as DSC_{:04d}.CR3 (a plain {} now gives DSC_1.CR3) - image_reading: read_channel_data_and_mask_from_img and the black_level_per_channel option use _get_reader and _read_or_raise, so unreadable images raise CorruptedImageError in step 3 as well - file_handling: the 9 and 23 column layouts are added to the upstream layouts - tests: the test library of upstream (NOISE_SEED) is used, the side band suite is renumbered to 10, the unit tests name the encoding of all text I/O and follow CorruptedImageError and _load_time_map(img_data)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merges upstream
master(e82e738) into the fork. It brings in the Windows port (FireDynamics#139), the handling of corrupted images (FireDynamics#137), the image name fix inConfigData(FireDynamics#142) and skipped images in the stacked analysis (FireDynamics#144). The fork features now follow the upstream conventions, so they can be offered upstream as separate PRs.Adaptations
format_img_nameis dropped. The ID is cast tointbefore formatting, as upstream does.image_reading:read_channel_data_and_mask_from_imgand theblack_level_per_channeloption use upstream's_get_readerand_read_or_raise. Unreadable images therefore also raiseCorruptedImageErrorin step 3 and are skipped.file_handling: the 9 and 23 column layouts are added to the upstream layouts, including upstream's hint for unparsable fitted files.ConfigDataStacked: the reference image options are kept; the config is now loaded asutf-8-sig.NOISE_SEED) is used.10_test_sideband_background.robot.test_raw_format_compatnow checksCorruptedImageErrorand extensions in either case._load_time_map(img_data)from leave images without extraction results out of the stacked analysis FireDynamics/LEDSmokeAnalysis#144.Behaviour changes
{}no longer pads the ID:DSC_{}.CR3now givesDSC_1.CR3instead ofDSC_0001.CR3. UseDSC_{:04d}.CR3.Checks
All checks ran on Windows with
cp1252and without UTF-8 mode.NOISE_SEED. Sidebg rmse is 0.014–0.027, against bounds of 0.05 and 0.06.sum_col_valandbgsub_sum_col_val(linear and nonlinear) are bit-identical. So are the 6 upstream columns of the step 3 files.mastervs. this branch, same test data:mastervs. this branch, three Hasenheide raw images in all three channels: the step 3 files and the extinction coefficients of all four reference properties are bit-identical.