Skip to content
This repository has been archived by the owner on Oct 8, 2019. It is now read-only.

Made spatial resolution an argument #33

Merged
merged 2 commits into from
May 25, 2012
Merged

Conversation

robinkraft
Copy link
Contributor

Tweaked static and rain preprocessing main functions to take spatial resolution as an argument.

@eightysteele
Copy link
Contributor

Maybe add a precondition that s-res is a string. Otherwise LGTM.

@robinkraft
Copy link
Contributor Author

Per this page, "arguments will be provided to your program as a seq of strings bound to the var command-line-args". So under no circumstances should an argument come in as anything but a string.

But do you think it's worth making it explicit, for the sake of future readers?

@eightysteele
Copy link
Contributor

Yeah you're right. Preconditions not needed. Merge.

@sritchie
Copy link
Contributor

Actually, I think a precondition is a great idea. You can still call the defmain function from the repl.

Also, we never use the command-line-args var, so that doesn't apply here. It's still true that arguments from the command line show up as strings, just not through that var.

@robinkraft
Copy link
Contributor Author

Good point. Will do.

Can you remind me btw why s-res and t-res have to be strings? I remember there was a justification at one point but have long since forgotten it.

On May 25, 2012, at 8:47 AM, Sam Ritchie reply@reply.github.com wrote:

Actually, I think a precondition is a great idea. You can still call the defmain function from the repl.

Also, we never use the command-line-args var, so that doesn't apply here. It's still true that arguments from the command line show up as strings, just not through that var.


Reply to this email directly or view it on GitHub:
#33 (comment)

@sritchie
Copy link
Contributor

Because they're strings in the schema, and because they're not really numeric values -- we're never going to perform any operations on them -- they're really just identifiers.

@eightysteele
Copy link
Contributor

Aaaaand I take it back. Preconditions are a great idea. :)

I wonder if something like :tres-500 and :sres-16 would be better than passing around strings.

@sritchie
Copy link
Contributor

I don't like that... then you'd have a settings map represented as {:tres :tres-500}. No need to add the field name into the option. (Also, you can't put keywords in thrift structs).

Much better to just have validation on the values. The right way to do this is to use a thrift enum.

enum Source {
  BACKTYPE = 1,
  TWITTER = 2,
  UNKNOWN = 3,
  FACEBOOK = 4,
  GOOGLE = 5,
  FOURSQUARE = 6,
  GOWALLA = 7,
  ALEXA = 8,
  DIGG = 9,
  RAPLEAF = 10,
  STUMBLEUPON = 11,
  YOUTUBE = 12,
  BITLY = 13
}

@eightysteele
Copy link
Contributor

Nice. Submitted issue for this:

#39

@sritchie
Copy link
Contributor

@robinkraft, if you add that precondition I'll merge this.

@robinkraft
Copy link
Contributor Author

68fd9a4 Added preconditions for spatial res as string

@sritchie sritchie merged this pull request into develop May 25, 2012
@robinkraft
Copy link
Contributor Author

Looks like this pull request was reverted - the resolutions are still hardcoded in develop as of today, with the most recent commit back in February.

https://github.com/reddmetrics/forma-clj/blob/e9f51d00db78ff79c2eecf3c74fbddfe9c05ce0a/src/clj/forma/hadoop/jobs/preprocess.clj#L24

Very strange ... I'll resubmit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants