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

WIP: Variable ocean levels #134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tcld
Copy link
Contributor

@tcld tcld commented Oct 15, 2015

Continuation from here: #125

A small fix to one of the test-files is still missing. I just want to put this here so the tests can have a go (on my system everything except the "ocean_level == serialized_ocean_leve?"-test worked fine) and maybe somebody else can look if I completely misunderstood things.
Maybe it would be appropriate to rename the variable "ocean_level" to "ocean_area" or "ocean_surface". But I understand that would be a bit of work.

@psi29a
Copy link
Member

psi29a commented Oct 15, 2015

Hahahahahhaha....

AssertionError: 0.65 != 0.6499999761581421

it is shit like this that makes me face palm so hard...

try assertAlmostEqual.

@tcld
Copy link
Contributor Author

tcld commented Oct 15, 2015

Good call.^^

EDIT: Problem with this is that the ocean is filled up by fill_ocean() and that is not an exact science. I gave it a little wiggle-room when trying to find the optimal ocean height since I didn't want to run into problems with varying floats there.

EDIT2: A failed test again. This one I have no clue about. Could this be a github hiccup?

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

AssertionError: <worldengine.world.World object at 0x7f970b748110> != <worldengine.world.World object at 0x7f970af2ed10>

You are comparing two different objects, so of course it fails. So you'll either need to compare the data members of the two objects or you'll need do the Python equivalent of == overload in the World class. Something like:

def __eq__():
    something something comparing members

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

I know of this problem. I already said it somewhere else, but I need to make some small changes that are already touched in the perf_opt branch. It would probably cause merge-problems if I did that twice. So for now the tests won't be passed.

(I don't have much experience with git, there might be a way around this.)

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

Can't you just merge the two branches?

Or, if you are ready... I can merge in your perf_opt branch and you can rebase this one on master.

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

Like this? Then I would have to close the pull request again, wouldn't I?

-master
     \
      \
        perf_opt -- ocean_level

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

Not at all!

We can do two things:

  1. You do a rebase of ocean_level based on perf_opt.
git checkout ocean_level
git rebase perf_opt
git push --force
  1. I merge in perf_opt. You do a:
git checkout master
git pull
git checkout ocean_level
git rebase master
git push --force

Either way is good, it is up to you. :)

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

Mmh, that looks reasonable. But, of course, I would prefer option 2. :P
(And thank you for the instructions. :) )

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

Right, give me a few and I'll give it a good eye-ball.

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

A few what?

This is perf_opt, by the way: #132

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

minutes/hours ;)

I'm also working atm...

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

I'm going to buy into approach 1, then. No unnecessary hurry.

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

Sure, that works too.

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

I tried this:

git checkout ocean_level
git rebase perf_opt
git push --force

Now I am curious what happens. On my system all tests passed etc.

EDIT: Mmh, this pull request now includes all the commits of the perf_opt-branch, too. I want to see how that plays out.

EDIT2: Yay, the checks are finally passed!

@psi29a
Copy link
Member

psi29a commented Oct 16, 2015

:)

What will happen is that I'll merge in the perf_opt when it is ready.

This pull request (github) is smart enough to know that those commits already exist in master and will only merge in the new commits into master when this branch is ready.

The only problem (small one, really) is if you push a new commit to the perf_opt branch. Then you'll just have to rebase this branch against perf_opt again. No big deal.

@tcld
Copy link
Contributor Author

tcld commented Oct 16, 2015

I won't push anything there. It's perfect. ;P

@psi29a
Copy link
Member

psi29a commented Oct 17, 2015

Indeed. :P It was merged without prejudice.

@psi29a
Copy link
Member

psi29a commented Oct 17, 2015

You still working here?

@psi29a
Copy link
Member

psi29a commented Oct 17, 2015

I'm rebuilding so that it uses the newly merged stuff in master.

@tcld
Copy link
Contributor Author

tcld commented Oct 17, 2015

I am currently testing something, better wait a bit with a merge (at least an hour).

After I did some more work yesterday I generated a really big map (3000x2000) and it had some artifacts on it. I now have to find out which commit introduced those (highly expect it to be the last one, but I still want to recheck this ocean_level-branch).

I will tell you once that test is done and how to proceed. :)

@tcld tcld mentioned this pull request Oct 17, 2015
@tcld
Copy link
Contributor Author

tcld commented Oct 17, 2015

My latest test still had some artifacts. I will start pulling the latest changes to worldengine, rebase this branch and try some more things.

@tcld
Copy link
Contributor Author

tcld commented Oct 17, 2015

So here is my problem, maybe somebody can help:
Two maps generated with the same seed, one using the current master-branch, one using the ocean_level_fix-branch. The latter one will look different due to this commit tcld@3e955e2
(platecs output depends on the sea_level-parameter it is passed, i.e. the amount of surface covered with water.)

The first image has an ocean-coverage of ~78% (used Gimp to find that out). For the second one that's ~71%.
seed_13050_elevation
seed_13050_elevation

The second map is ugly, and I am not even sure if it is due to a problem in the code. When draining parts of the ocean, certain spots of the elevation map will start to show up as land. And even the first image shows that there are many shallow places in the ocean that would probably be above water were the ocean a little less deep.

Maybe somebody can give some input about this. Meanwhile I may cherry-pick the commits of this branch that are useful for the numpy-branch so it can be merged in before the ocean-levels. If no better solution is found.

@tcld
Copy link
Contributor Author

tcld commented Oct 17, 2015

I just realized something: I was using 9 plates for platec. That must have been the reason for the land-masses being distributed so equally. Here two images (master then ocean_level_fix) for 3 plates:

seed_13051_elevation
seed_13051_elevation

Maybe the problem is not a problem? Maybe a planet cramped with plates will have a lot of islands?

@tcld
Copy link
Contributor Author

tcld commented Oct 17, 2015

Right now this branch is most likely incompatible with the numpy-changes. Should the "problems" above find their solution, I will rebase this branch and make everything nice and clean.

EDIT: Rebasing is done, the actual changes this branch introduces are very few.

@tcld tcld changed the title Variable ocean levels WIP: Variable ocean levels Nov 12, 2015
@tcld
Copy link
Contributor Author

tcld commented Nov 17, 2015

Rebased, now the list of changes is really short. I didn't check the output, though, since I expect the previous problem to still exist.

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.

None yet

2 participants