Limit "no-damage" value to available values from selected "no-damage" column when possible#474
Open
Limit "no-damage" value to available values from selected "no-damage" column when possible#474
Conversation
This makes it easier to understand why an asset has an error. In real life, an asset had non-ascii characters in the column name, so add a specific check for that, because when the character is non-printable, it's impossible to tell with the naked eye why the name is wrong.
This commit introduces dynamic behavior for the 'no-damage value' input field on the disaster management page. Key changes: - The 'no-damage value' field now conditionally renders as either a dropdown or a free-form text input. - If the selected 'no-damage column' in the damage asset has 25 or fewer unique values, a dropdown populated with these values is displayed. - If the column has more than 25 unique values, or no column is selected, a free-form text input is displayed. - The 'no-damage value' (whether from the dropdown or input) is cleared if the main damage asset is changed. - The `getExemplars` function in `add_layer.js` was confirmed to be suitable for fetching unique column values. - Unit tests have been added in `cypress/integration/unit_tests/manage_disaster_damage_logic_test.js` to cover the new conditional rendering, data saving, and value clearing logic. This enhancement improves your experience by providing a guided selection for the 'no-damage value' when the number of choices is manageable.
The `getExemplars` function in `docs/import/add_layer.js` was incorrectly attempting to use `featureCollection.aggregate_count_distinct(property)`. This is not a valid Earth Engine function call in that context and resulted in a runtime error: "Uncaught (in promise) TypeError: featureCollection.aggregate_count_distinct is not a function". This commit corrects the implementation to use `featureCollection.aggregate_histogram(property)`. The number of distinct values is then determined by `ee.Dictionary(histogram).keys().length()`. This provides the correct count of unique values for the property. The rest of the logic in `getExemplars` (i.e., returning the list of unique values if their count is <= 25, or an empty list otherwise) remains the same. The dependent logic in `manage_disaster_base.js` and the corresponding unit tests in `cypress/integration/unit_tests/manage_disaster_damage_logic_test.js` were reviewed and confirmed to be compatible with this corrected implementation without requiring further changes.
This commit addresses a persistent issue in the `getExemplars` function
in `docs/import/add_layer.js`. Previous attempts to fix the Earth
Engine calls were still incorrect.
The function now correctly uses
`featureCollection.reduceColumns({reducer: ee.Reducer.frequencyHistogram(), selectors: [property]})`
to obtain a frequency histogram for the specified property. The distinct
values and their count are then derived from this histogram.
This replaces the previous erroneous attempts to use
`featureCollection.aggregate_count_distinct(property)` or
`featureCollection.aggregate_histogram(property)` directly, which are
not valid methods for this purpose in the Earth Engine JavaScript API.
The core logic of `getExemplars`—returning a list of distinct values
if their count is 25 or less, or an empty list otherwise—remains
unchanged.
Dependent code in `manage_disaster_base.js` and the unit tests in
`cypress/integration/unit_tests/manage_disaster_damage_logic_test.js`
have been reviewed and confirmed to be compatible with this corrected
implementation, as they rely on the output contract of `getExemplars`
rather than its internal EE API calls.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the “no-damage” input by populating a dropdown of actual values from the selected damage column (falling back to text input if there are too many values), pre-fetches unique values for all columns, and updates related logic and tests.
- Pre-fetch unique values for each damage column using a new
getExemplarshelper and store inpropertyValues. - Refactor
setNoDamageColumnAndValueandmaybeShowNoDamageValueItemto handle pending states and toggle between select/input. - Adjust Cypress tests to use the new select element ID suffix (
-select) and updated exemplars behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docs/import/manage_disaster_base.js | Added logic to fetch and cache unique damage values, refactored column/value toggling |
| docs/import/add_layer.js | Extracted getExemplars helper and replaced inline histogram code |
| cypress/integration/unit_tests/manage_disaster_set_score_parameters_test.js | Updated tests for new select ID suffix and exemplars data patterns |
Comments suppressed due to low confidence (1)
docs/import/add_layer.js:13
- [nitpick] Refine the JSDoc for
getExemplars: specify that thepropertyparameter is a string key of the feature attribute and clarify the precise return type (ee.Listoree.Object).
* @param {Object} property property
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.
This resolves a longstanding pain point/TODO: when the damage asset contains both damaged and undamaged buildings (likely, all buildings in the area are listed), how do we specify which buildings are damaged. It can be a value like "0", or "no damage", etc. Previously, the entry field was always free text, so a typo would mean that all buildings were considered damaged. Now we eagerly fetch the values from the columns of the damage asset so that when the user selects a "no-damage" column, we populate a select drop-down with the potential values, avoiding errors.
When there are more than 25 distinct values in a column (for instance, if the damage value is given as a percentage, with 0 being undamaged), the select is not created, and we revert to the normal text input field.
The value of the field is shared across the select and the text input, and is preserved across column changes/asset changes, since it's more likely than not that the user will upload a newer version of the asset, with the same desired values.
The main behavior change is to kick off fetches for the values of all the columns of the damage asset once we have the values of the columns themselves. This isn't the most efficient thing to do end-to-end-latency-wise: we could fetch the columns and values at the same time. However, that would mean we wouldn't get the columns until the very end, and it's helpful to populate as we go.
setNoDamageColumnAndValue now gets the columns (and values) from a static map, so it gets an argument of whether the columns are ready, as well as whether there's a damage asset at all.