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

Geo histogram (#230) #231

Closed
wants to merge 8 commits into from
Closed

Geo histogram (#230) #231

wants to merge 8 commits into from

Conversation

ababaian
Copy link
Member

Pardon my misuse of git if this isn't the ideal way to do this, but to get the geo-histogram branch from @khanzardar onto this repo as it's own branch (not merged into main). I accepted it onto this geo_time branch and now am opening a PR to go from serratus.io:geo_time into serratus.io:main.

  • timeline component fetching .tsv data and attempting to create histogram. Not plotting in monthly increments atm.

  • working histogram, plots by month, using d3's max/min to find date ranges

  • Able to pass selectedPoints to TimePlot component, if selectedPoints histogram will adjust to plot on that data - not working flawlessly

  • Histogram labels added, histogram will keep full data & allow overlay of selected data

  • x-axis will now remain static based off of entire dataset

  • updated TODO comment

  • fixed prettier warning by running npm command


Summary of changes

Related issues

Checklist

  • Add tests or provide evidence of manual testing (if applicable)
  • PR checks pass (see CONTRIBUTING.md for how to fix failing checks)

@ababaian ababaian mentioned this pull request Feb 20, 2023
2 tasks
@ababaian
Copy link
Member Author

So not building at the moment :*(

@khanzardar
Copy link
Contributor

I will take a look into why build is failing.

@khanzardar
Copy link
Contributor

khanzardar commented Feb 22, 2023

Hey @ababaian, it's currently unclear to me why this build is failing. I do see the following error message in the logs:

/usr/bin/git ls-remote -h -t ssh://git@github.com/rbuels/librpc-web.git
2023-02-20T18:12:09.6980002Z npm ERR! 
2023-02-20T18:12:09.6983076Z npm ERR! git@github.com: Permission denied (publickey).
2023-02-20T18:12:09.6986466Z npm ERR! fatal: Could not read from remote repository.
2023-02-20T18:12:09.6989672Z npm ERR! 
2023-02-20T18:12:09.6992780Z npm ERR! Please make sure you have the correct access rights
2023-02-20T18:12:09.6996112Z npm ERR! and the repository exists.

Have you run into this before?

EDIT:
The issue appears to be around librpc-web dependency. I'm unable to reproduce on local by running npm install or npm ci. However I noticed that I running node v16 on local whereas the worker's running on node v14. I will switch local to v14 to see if I can replicate.

@ababaian
Copy link
Member Author

We keep running into this, need to pin the dependencies for local builds so it matches worker. Or update worker? @victorlin might have some better insight on the optimal choice here.

@khanzardar
Copy link
Contributor

khanzardar commented Feb 22, 2023

@ababaian Okay, I'm able to reproduce error seen by worker on local once I switched to node v14. Appears to be an issue linked with cloning a repo over ssh. See the following link for more information: npm/cli#2610.

I'm all ears to any insights you may have. In the meantime, I'll follow the thread above to see possible workaround / solutions.

EDIT:
Based on @victorlin's suggestion in the above linked issue. I'm able to revert to lock v1 npm install --package-lock-only and can run npm ci, npm build successfully. Your call on how to proceed.

@ababaian
Copy link
Member Author

How do we update the worker build?

@victorlin
Copy link
Member

@ababaian: Pardon my misuse of git if this isn't the ideal way to do this, but to get the #229 branch from @khanzardar onto this repo as it's own branch (not merged into main). I accepted it onto this geo_time branch and now am opening a PR to go from serratus.io:geo_time into serratus.io:main.

This sounds reasonable to me (see #232).

@khanzardar: However I noticed that I running node v16 on local whereas the worker's running on node v14
@ababaian: How do we update the worker build?

Yeah, this project was built on v14 and both the CI workflow (on GitHub) and the deployed builds (on AWS Amplify) use v14. I do think that we should update to v16 soon though. This was started in #195 but need to dig deeper into build issues.

@khanzardar
Copy link
Contributor

I've pushed the updated branch - with reversion to package-lock v1 - onto my forked repo & made a PR to merge into this repo's geo_time. If you agree, we can merge these changes into main to temporary deal with this issue until we decide to upgrade to node v16.

@ababaian
Copy link
Member Author

I merged the commit, but still not building

@victorlin
Copy link
Member

I'm going to merge #195 and update this PR to work with that.

@ababaian
Copy link
Member Author

OK Live version is now available at: https://geo-time.serratus.io/ Sorry about the delay

Would you be able to make the width of the plot adjust to the window size so it's say 90% width of the iframe?

@ababaian
Copy link
Member Author

@khanzardar bump

@khanzardar
Copy link
Contributor

Sweet, will do today!

@khanzardar khanzardar mentioned this pull request Feb 28, 2023
1 task
* Geo histogram (#230)

* timeline component fetching .tsv data and attempting to create histogram. Not plotting in monthly increments atm.

* working histogram, plots by month, using d3's max/min to find date ranges

* Able to pass selectedPoints to TimePlot component, if selectedPoints histogram will adjust to plot on that data - not working flawlessly

* Histogram labels added, histogram will keep full data & allow overlay of selected data

* x-axis will now remain static based off of entire dataset

* updated TODO comment

* fixed prettier warning by running npm command

---------

Co-authored-by: zardarATgojitech <zardar.khan@gojitech.co>

* reverting package-lock to v1 to avoid ssh issue re: dependency repo

* updated TimelinePlot component to autosize

---------

Co-authored-by: Artem Babaian <ababaian@bccrc.ca>
Co-authored-by: zardarATgojitech <zardar.khan@gojitech.co>
lukepereira added a commit that referenced this pull request Mar 27, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 27, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 27, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 27, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 28, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 28, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 28, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 28, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 28, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
@lukepereira
Copy link
Collaborator

closing in favour of #239

lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
lukepereira added a commit that referenced this pull request Mar 29, 2023
- Merge #221
- Merge #231

Co-authored-by: Max McCready <75914260+Bluesquare99@users.noreply.github.com>
Co-authored-by: khanzardar <k.zardar@gmail.com>
@ababaian ababaian deleted the geo_time branch May 4, 2023 16:44
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.

4 participants