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

Fix Batch AOI stats #139

Merged
merged 15 commits into from
Mar 11, 2024
Merged

Fix Batch AOI stats #139

merged 15 commits into from
Mar 11, 2024

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Mar 6, 2024

Contributes to #132.

Changes:

  • Display stats of batch AOIs in the secondary panel
  • Disable prime button if a prediction was already made for AOI, Mosaic & Checkpoint combination
  • Disable prime button if batch is already running
  • Never display Class distribution metrics are not available, hide the panel instead

@LanesGood this is ready for review. To test, try opening a finished batch AOI, switch between AOIs and confirm the stats are displayed accordingly.

@vgeorge vgeorge requested a review from LanesGood March 6, 2024 13:15
@LanesGood
Copy link
Member

@vgeorge I'm a bit confused. I tested with a new batch prediction - please see the below.

Some minor changes perhaps could be useful:

  • I can see that the "Run batch prediction" button is still active when my currently selected batch is not yet running. (Strangely the "processing" notification never activated, but the prediction finished.)
image

My batch prediction returned, but the secondary stats panel doesn't load:
image

Reloading the browser window, I can see the class stats should be there:
image

Initially when I reloaded, the secondary panel didn't appear. My screenshot for this didn't save. I'll try to reproduce.

Reloaded after new AOI:

  • I can properly see the Class Stats
image

</Prose>
</ClassItem>
)}
<ClassItem>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional check for bounds (or totalArea, whatever we want to call it) should remain; this ensures that the table header is used only for the class distribution chart and not the class F1 chart. Here's the view in this branch with the current code:
image

{landArea(checkpoint.analytics[i][metric])}
</Prose>
)}
<Prose size='small' className='percent'>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep conditional check - only need this column of data on the class distribution chart, but the component is also used for class F1 scores

@LanesGood
Copy link
Member

@vgeorge I added a change to keep the land area in the compare and share pages' class analytics charts. Not sure if best practice to execute the areaFromBounds function in the chart component, or before passing the prop through; I did it in the component. Will defer to you on the conditional check - I'm not sure if we should separate out the component logic more clearly or if the original check I had implemented is adequate.

@vgeorge
Copy link
Member Author

vgeorge commented Mar 8, 2024

@LanesGood I added changes on how the batch status is controlled internally. Now there is a specific machine to control its state. I haven't gotten to the issues you reported, but it would be nice if you could review this again, specifically the batch panel behaviour.

@vgeorge vgeorge requested a review from LanesGood March 8, 2024 17:31
Copy link
Member

@LanesGood LanesGood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I see the "Batch prediction starting" button
    image

  2. I see the batch progress in the button! Remarkably fast, so while I saw the progress increasing I didn't actually snap a screenshot of it in time. 🎉

  3. Batch finishes but does not render 😞
    image
    Console of state machine exposes this: ckpt: undefined | aoi: 145 | tmf: undefined | msc: a1b11210f9ecc3fdc461c9379d6f0d53

Reloading, I can see the batch predictions:
image

@vgeorge I think we should call this PR good - it achieved the "fix batch aoi stats" goal and is successful. The rendering is something else. I also think that the expected behavior is usually that users start a batch and return to that page later, so that may be why the immediate rendering of the batch AOI predictions is not inherent already.

I do see that the "run batch prediction" button is still active on a selected AOI when nothing has changed - this isn't captured by your test cases but perhaps we can flag for future enhancement,

vgeorge and others added 4 commits March 11, 2024 11:04
…s-chart.js

Co-authored-by: Lane Goodman <lane@developmentseed.org>
…s-chart.js

Co-authored-by: Lane Goodman <lane@developmentseed.org>
…s-chart.js

Co-authored-by: Lane Goodman <lane@developmentseed.org>
@vgeorge
Copy link
Member Author

vgeorge commented Mar 11, 2024

@LanesGood thanks for the review! I've incorporated some of your suggested changes and added a commit to fix the styles in the secondary panel. I'm going to merge this now. Let's track any new issues in the panel with separate tickets.

@vgeorge vgeorge merged commit cc46347 into develop Mar 11, 2024
3 checks passed
@vgeorge vgeorge deleted the fix/batch-aoi branch March 11, 2024 12:15
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.

None yet

2 participants