-
Notifications
You must be signed in to change notification settings - Fork 4
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
simplify run_export #6
Conversation
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.
Noting specific API changes
@@ -25,16 +25,16 @@ def self.delete_temp_files(path) | |||
## Run on each image: | |||
|
|||
# pixels per meter = pxperm | |||
def self.generate_perspectival_distort(pxperm, path, nodes_array, id, image_file_name, img_url, height, width, root = "https://mapknitter.org") | |||
def self.generate_perspectival_distort(pxperm, id, nodes_array, image_file_name, img_url, height, width, root = "https://mapknitter.org") |
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.
Change 1
system(self.ulimit+imageMagick) | ||
end | ||
|
||
# runs the above map functions while maintaining a record of state in an Export model; | ||
# we'll be replacing the export model state with a flat status file | ||
def self.run_export(user_id, resolution, export, id, slug, root, average_scale, placed_warpables, key) | ||
def self.run_export(user_id, resolution, export, id, root, placed_warpables, key) |
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.
Change 2
root, | ||
scale, | ||
[image], # TODO: these images need a nodes_array | ||
[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.
Updated test
|
||
coords = MapKnitterExporter.generate_perspectival_distort( | ||
scale, | ||
slug, | ||
id, |
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.
Updated test
Getting stuck where it can't find the output of the previous file...
I fixed a few but have to step away; @icarito would you be able to take this forward by looking at the output of each step as it goes to the next? |
Now let's rebase this and complete the simplification... |
turned slug into id (will need test fixes), removed average_scale Simplified `run_export` to `def self.run_export(user_id, resolution, export, id, root, placed_warpables, key)`
attempted a rebase at #9, but kept it separate just to compare to be sure i've done it right. |
OK, the rebase was good; force pushing it here and closing the other PR: #9 |
4ca193b
to
e7ccbd5
Compare
Hmm, this looks like an issue with the
|
Getting there!
|
Somehow this is failing on line 85:
Hmm. I'll log out the vars. |
There is a fix for this in main branch - using ipv4 pool (ipv4.pool.sks-keyservers.net) instead! |
Ah sorry that fix is in mapknitter main repo |
😄
|
Now:
|
|
||
# get rid of existing geotiff | ||
system("rm -r public/warps/#{slug}/1-geo.tif") | ||
system("rm -r public/warps/#{id}/1-geo.tif") |
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.
we should change all 1
to #{id}
and set id
to 2 for this run.
Also noting |
Well, there we go. There are some issues to resolve, though:
And i'm not sure why the test doesn't catch that - any ideas, @icarito? |
Some of these warnings about not being able to delete things are because some of the methods try to delete anything in the location before trying to make something in that location, and I think that's OK to leave:
|
I think this is the last critical bug to resolve --
It's because the final command still runs against |
But, if this passes, we can merge it! |
I think we need to address and simplify the usage of "root" -- is it even relevant anymore for how it's structured now? I think it originally was trying to use Rails.root but now we're mostly using it locally. I fixed it again. We can do "root" cleanup in a follow-up PR. |
Merge once we resolve everything and after #4; also refactor https://github.com/publiclab/mapknitter/ to use this, and make downstream adjustments in https://github.com/publiclab/mapknitter-exporter-sinatra/
Changes:
turned
slug
intoid
(will need test fixes), removedaverage_scale
def self.run_export(user_id, resolution, export, id, slug, root, average_scale, placed_warpables, key)
becamedef self.run_export(user_id, resolution, export, id, root, placed_warpables, key)
def self.generate_perspectival_distort(pxperm, path, nodes_array, id, image_file_name, img_url, height, width, root = "https://mapknitter.org")
becamedef self.generate_perspectival_distort(pxperm, id, nodes_array, image_file_name, img_url, height, width, root = "https://mapknitter.org")