-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
Great to see this! 🚀 |
…d, scale f32 values to be 0-1
@Bushuo There is an error in storybook. It seems to also scan the generated definitions file which contains an example of the geometry in json. Would you mind fixing that? Probably the best solution would be to exclude the definitions.ts from storybook somehow, if that is not possible I don't really know how to handle that. |
@kitzbergerg Hi, I looked at it. |
@markus2330 This is now finished I would say. It is a bit difficult to test as many things are not yet in place, but I will provide some scripts that can be used. @markus2330 @Bushuo Here are a few issues/implementation details that appeared related to this:
StepsHere are the steps I used to test the heatmap:
curl --location 'http://localhost:8080/api/maps' \
--header 'Authorization: Bearer <token>' \
--header 'Content-Type: application/json' \
--data '{
"name": "Test1",
"creation_date": "2023-07-06",
"is_inactive": false,
"zoom_factor": 100,
"honors": 0,
"visits": 0,
"harvested": 0,
"privacy": "public",
"description": "",
"geometry": {
"rings": [
[
{
"x": 0.0,
"y": 0.0
},
{
"x": 100.0,
"y": 0.0
},
{
"x": 100.0,
"y": 100.0
},
{
"x": 50.0,
"y": 100.0
},
{
"x": 50.0,
"y": 50.0
},
{
"x": 0.0,
"y": 50.0
},
{
"x": 0.0,
"y": 0.0
}
]
],
"srid": 4326
}
}'
INSERT INTO relations
VALUES (1, 2, 'companion'),
(1, 3, 'neutral'),
(1, 4, 'antagonist');
INSERT INTO plantings
VALUES ('00000000-0000-0000-0000-000000000000', 2, 1, 10, 10, 0, 0, 0, 0, 0),
('00000000-0000-0000-0000-000000000001', 2, 2, 11, 10, 0, 0, 0, 0, 0),
('00000000-0000-0000-0000-000000000002', 2, 2, 23, 10, 0, 0, 0, 0, 0),
('00000000-0000-0000-0000-000000000003', 2, 2, 40, 10, 0, 0, 0, 0, 0),
('00000000-0000-0000-0000-000000000004', 2, 2, 10, 20, 0, 0, 0, 0, 0),
('00000000-0000-0000-0000-000000000005', 2, 3, 10, 20, 0, 0, 0, 0, 0);
curl --location 'http://localhost:8080/api/maps/1/layers/plants/heatmap?plant_id=1&layer_id=2' \
--header 'Authorization: Bearer <token>' |
Yes, placement is indeed currently wrong. We changed the plant from a square to a circle which broke it. Should this be the bottom left of the planting? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly a big step forwards, great to have the endpoint. I would prefer integer used more. Documentation etc. is a bit weak. Thanks for adding the test protocol, it now worked for me but I couldn't reproduce it with self-planted plantings. Probably a bigger map is needed for that (it should be 1ha per default anyways).
About the (not previously discussed) implementation in plpgsql (we actually said it should be in Rust or directly in a SQL query using PostGIS) I am not sure. The main concern is the performance: please check it as part of your thesis.
@@ -0,0 +1,104 @@ | |||
# Test Protocol Heatmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link test protocol from use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this is supposed to mean. There is no test protocol for the use case in plant layer.
Should I link to the usecase? Something like ../usecases/current/plants_layer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the use case (plants layer), there should be a link to this test protocol doc/tests/protocols/MTP_2023-07-8_Heatmap_V1.md.
@@ -127,6 +127,20 @@ export default function MapCreateForm() { | |||
privacy: mapInput.privacy, | |||
description: mapInput.description, | |||
location: !Number.isNaN(mapInput.location.latitude) ? mapInput.location : undefined, | |||
// TODO: implement selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe better what you mean here. Do you mean #494, then please link to the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a distinction between these two issues (one for backend, other for frontend).
Simply said when a map is created the user needs to specify the boundaries of the map. This boundary is then used by the heatmap (and maybe other layers like the base layer?).
Please read the use cases, the map needs to be created without the boundaries. So it must be possible to set a boundary afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the map boundary be part of the base layer or part of the map? As far as I understand the following means it is part of map: these boarders are stored in the map and not subjective to "alternatives"
(for the base_layer use case).
If so that would mean I simply need to set the field to nullable and add an option to update the boundary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the map boundary must be part of the map.
Usually the left-over issues should be created by the implementer of a PR.
|
jenkins build please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well done.
@markus2330 Just one question:
How often is this heatmap supposed to be updated?
As it is now, it seems to be quite resource intensive to generate.
entity::plant_layer, | ||
}, | ||
}; | ||
|
||
/// Generates a heatmap signaling ideal locations for planting the plant. | ||
/// The return values are raw bytes of an PNG image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The return values are raw bytes of an PNG image. | |
/// The return values are raw bytes of a PNG image. |
It always gets generated freshly, as it is different for every plant and after basically every change in the map.
Is this just your general feeling or do have specific concerns? |
Implementation of the backends heatmap.
Resolves #457, resolves #635, resolves #494.
Basics
close #X
, are in the commit messages.Checklist
(not in the PR description)
(exceptions are documented)
Review