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

Interface & Visualization #50

Merged
merged 15 commits into from
Jul 24, 2020
Merged

Conversation

im-prakher
Copy link
Contributor

No description provided.

@KristinaRiemer
Copy link
Contributor

So I was trying to follow along with the interface README. When I ran yarn start, it returned this error: Command failed with exit code 127. After some Googling, apparently just yarn needs to be run first, and then yarn start worked. So that should be added to the README.

This is how the first page looks for me:
Screen Shot 2020-06-26 at 8 23 23 AM

@KristinaRiemer KristinaRiemer changed the title Interface & Visualization WIP Interface & Visualization Jun 26, 2020
Added install dependencies section
Copy link
Contributor

@Chris-Schnaufer Chris-Schnaufer left a comment

Choose a reason for hiding this comment

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

Can you comment where you're using generated files vs. ones that you created and/or modified. I think this would help reviewers.
Adding comments to files as to the intent of the file would help people understand what they're about. Especially for example, upload1.js and upload2.js.
Python documentation strings are missing in python code

app/Meta.py Show resolved Hide resolved
app/Meta.py Outdated
for pt in list(row['geometry'].exterior.coords):
pt=pt+(115,)
flat_list.append(pt)
poly = Polygon(flat_list)
data.loc[index, 'geometry'] = poly

if(all(x in accepted_columns for x in columns)):
data['notes']=data_g['notes']
#data['geometry']=data_g['geometry']
data['time_zone'].fillna("America/Phoenix", inplace = True)
data['soilnotes'].fillna("some text", inplace = True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel the term "some text" should be replaced with "No notes". I feel that this is clearer

app/Meta.py Show resolved Hide resolved
app/Meta.py Outdated
@@ -191,8 +181,7 @@ def insert_sites(fileName,shp_file,dbf_file,prj_file,shx_file):
os.remove(shp_file_target)
os.remove(dbf_file_target)
os.remove(prj_file_target)
os.remove(shx_file_target)
os.remove(file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

os.remove(file_name) should remain here so that it's cleaned up when a problem occurs. Checking if the file exists before trying to remove it would prevent a missing file exception from being raised

if os.path.exists(file_name):
    os.remove(file_name)

Copy link
Contributor

@saurabh1969 saurabh1969 Jul 16, 2020

Choose a reason for hiding this comment

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

os.remove(file_name) should be there.Otherwise file contents will be appended with existing ones when that endpoint is called next time.

client/client.py Show resolved Hide resolved
}


const getTable1=async (cultivars, sites_cultivars, sites) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a more descriptive name here instead of "getTable1". Perhaps insertCultivarSites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! for pointing out, will change it in a future commit.

}
}

const getTable2=async (experiments, experiments_sites, sites) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a more descriptive name than "getTable2" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! for pointing out, will change it in a future commit.


sites.pop();
sites.pop();
sites.push("some text")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "No notes" here as well

await client.query(query2);

sites.pop();
sites.push("some text")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "No notes" here and everywhere else soilnotes defaults to "some text"

visualization/pg_joins.js Show resolved Hide resolved
Co-authored-by: Chris Schnaufer <schnaufer@email.arizona.edu>
app/Meta.py Outdated
for index, row in data_g.iterrows():
flat_list = []
for pt in list(row['geometry'].exterior.coords):
pt=pt+(115,)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line of code adds another dimension to the loaded point. This causes problems when the points already have a Z value when loaded from the shapefile (point has 3 dimensions); after this line the point will have 4 dimensions and cause a failure

Copy link
Contributor

Choose a reason for hiding this comment

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

Another comment on this is that every point that doesn't have a Z value will be assigned an elevation of 115 meters. IMO it would be better to default to zero or some other value indicating that it's probably not correct (unless you're at sea level)

app/Meta.py Outdated Show resolved Hide resolved
app/Meta.py Show resolved Hide resolved
Copy link
Member

@dlebauer dlebauer left a comment

Choose a reason for hiding this comment

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

  • @im-prakher try " " instead of no notes?
  • @Chris-Schnaufer could you provide more explicit instructions or go ahead and implement your suggestions below for .shp file and time zone handling? It isn't clear how challenging this would be but I think especially the shapefile handling could become a rabbit hole for someone not as familiar w/ GIS as you.

client/client.py Show resolved Hide resolved
interface/public/tw.svg Show resolved Hide resolved
await client.query(query2);

sites.pop();
sites.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sites.push("some text")
sites.push("")


sites.pop();
sites.pop();
sites.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sites.push("some text")
sites.push("")


sites.pop();
sites.pop();
sites.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sites.push("some text")
sites.push("")


sites.pop();
sites.pop();
sites.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sites.push("some text")
sites.push("")

site.pop()
site.pop()
site.pop()
site.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
site.push("some text")
site.push("")

site.pop()
site.pop()
site.pop()
site.push("some text")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
site.push("some text")
site.push("")

@KristinaRiemer
Copy link
Contributor

@dlebauer or @Chris-Schnaufer would you be able to confirm your review so that this PR can be merged?

@KristinaRiemer KristinaRiemer changed the title WIP Interface & Visualization Interface & Visualization Jul 17, 2020
app/Meta.py Outdated Show resolved Hide resolved
im-prakher and others added 8 commits July 18, 2020 20:59
@im-prakher
Copy link
Contributor Author

This PR is now complete in itself, please have a final review and then merge it.
@Chris-Schnaufer @KristinaRiemer @dlebauer @saurabh1969

@im-prakher
Copy link
Contributor Author

Due to a lot of lines of code in this PR, it might be challenging to review the work done in recent commits so to make this easier, I have opened issues(regarding the recent commits) that are implemented in this PR.


Before running the project, run:

### `yarn`
Copy link
Member

Choose a reason for hiding this comment

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

Use code blocks for indicating code, e.g.

yarn

Copy link
Member

Choose a reason for hiding this comment

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

app/Meta.py Show resolved Hide resolved
client/client.py Show resolved Hide resolved
interface/src/App.test.js Show resolved Hide resolved
visualization/pg_joins.js Show resolved Hide resolved
app/Meta.py Show resolved Hide resolved
@dlebauer dlebauer merged commit 3d86329 into PecanProject:master Jul 24, 2020
@im-prakher im-prakher deleted the interface branch February 1, 2021 10:36
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.

5 participants